8370666: VectorAPI: Add clear comments for vector mask relative code in c2 #29130

XiaohongGong · 2026-01-09T01:36:50Z

The VectorMask implementation in Vector API involves complex interactions between types, nodes, and platform-specific
features, making the related code in HotSpot difficult to understand and review.

This patch adds comprehensive comments for vector mask related types, nodes, and methods in C2 to improve code clarity and
maintainability.

Note: This patch only adds comments; no functional changes are made.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8370666: VectorAPI: Add clear comments for vector mask relative code in c2 (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/29130/head:pull/29130
$ git checkout pull/29130

Update a local copy of the PR:
$ git checkout pull/29130
$ git pull https://git.openjdk.org/jdk.git pull/29130/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 29130

View PR using the GUI difftool:
$ git pr show -t 29130

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/29130.diff

Using Webrev

Link to Webrev Comment

…in c2

bridgekeeper · 2026-01-09T01:37:55Z

👋 Welcome back xgong! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-01-09T01:39:31Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2026-01-09T01:40:17Z

@XiaohongGong The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2026-01-09T01:44:55Z

Webrevs

00: Full (ed5e79f7)

eme64

Nice work, thanks for taking the time for this, much appreciated!

On the whole I'm super happy with this, but left a few extra comments :)

eme64 · 2026-01-09T11:00:42Z

src/hotspot/share/opto/vectornode.hpp

+// is set for the masked version. Meanwhile, there is an additional mask input for
+// the masked nodes.
+//
+// For example, "AddVBNode" might have two versions:


Might?

Suggested change

// For example, "AddVBNode" might have two versions:

// For example:

eme64 · 2026-01-09T11:03:47Z

src/hotspot/share/opto/type.cpp

+// - Returns "TypeVectMask" for platforms that support the predicate feature and
+//   it is implemented properly in the backend, allowing the vector mask to be
+//   stored in a predicate/mask register.
+// - Returns a normal vector type (i.e. TypeVectA ~ TypeVectZ) otherwise, where
+//   the vector mask is stored in a vector register.


The first case is PVectMask, and the second NVectMask, right?

eme64 · 2026-01-09T13:19:43Z

src/hotspot/share/opto/vectornode.hpp

 public:
  VectorRearrangeNode(Node* vec1, Node* shuffle)
    : VectorNode(vec1, shuffle, vec1->bottom_type()->is_vect()) {
-    // assert(mask->is_VectorMask(), "VectorBlendNode requires that third argument be a mask");


Can you add a comment for Rearrange as well?

eme64 · 2026-01-09T13:25:17Z

src/hotspot/share/opto/vectornode.hpp


-
 // Select elements from two source vectors based on the wrapped indexes held in
 // the first vector.


Can you improve the documentation a little? Also: "first vector" might be understood to refer to src1, but that's not the case, right?

You could base the description on selectFrom:
https://download.java.net/java/early_access/jdk26/docs/api/jdk.incubator.vector/jdk/incubator/vector/FloatVector.html#selectFrom(jdk.incubator.vector.Vector,jdk.incubator.vector.Vector)

eme64 · 2026-01-09T13:26:52Z

src/hotspot/share/opto/vectornode.hpp

+// The target may not directly support the rearrange operation for an element type.
+// In those cases, we can transform the rearrange into a different element type.


Can you specify a bit more about the inputs and outputs, and what exactly the transformation does?

Is this a memory instruction? Because it is called Load. If not: can we rename it?

eme64 · 2026-01-09T13:30:15Z

src/hotspot/share/opto/vectornode.hpp

+// Convert a "BVectMask" into a platform-specific vector mask (either "NVectMask"
+// or "PVectMask").
 class VectorLoadMaskNode : public VectorNode {


I'd love to rename this. Because it is (as you say in the comments) a conversion, and not a "load" (memory op).
What about VectorConvertBooleans2MaskNode.

And below, rename VectorStoreMaskNode to VectorConvertMask2BooleansNode.

You may have an even better idea.

eme64 · 2026-01-09T13:32:15Z

src/hotspot/share/opto/vectornode.hpp

 };

-//-------------------------- Vector mask broadcast -----------------------------------
+//-------------------------- Vector mask broadcast ------------------------------


A nit that has been bothering me for a while:

I would just remove all the "title lines" with the ------.
They don't really add anything. And they are currently inconsistent in this file anyway.
Some are a more of a description like here. Some repeat the node name. And in some cases they are missing anyway.

8370666: VectorAPI: Add clear comments for vector mask relative code …

ed5e79f

…in c2

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jan 9, 2026

openjdk bot added the rfr Pull request is ready for review label Jan 9, 2026

eme64 reviewed Jan 9, 2026

View reviewed changes

	// For example, "AddVBNode" might have two versions:
	// For example:



		// Select elements from two source vectors based on the wrapped indexes held in
		// the first vector.

		// The target may not directly support the rearrange operation for an element type.
		// In those cases, we can transform the rearrange into a different element type.

8370666: VectorAPI: Add clear comments for vector mask relative code in c2 #29130

Are you sure you want to change the base?

8370666: VectorAPI: Add clear comments for vector mask relative code in c2 #29130

Conversation

XiaohongGong commented Jan 9, 2026 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Jan 9, 2026

Uh oh!

openjdk bot commented Jan 9, 2026

Uh oh!

openjdk bot commented Jan 9, 2026

Uh oh!

mlbridge bot commented Jan 9, 2026

Webrevs

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

XiaohongGong commented Jan 9, 2026 •

edited by openjdk bot

Loading