8384963: C2: Incorrect uint constant match mishandles negative values in vectors

[jatin-bhateja]

JDK-8341137 introduced a C2 pattern matcher (MulVLNode::has_uint_inputs) that lowers MulVL to x86 VPMULUDQ when both operands are provably representable in uint32, The constant bound was checked with a signed comparison against 0xFFFFFFFFL, which trivially accepts any negative jlong constant (-1L, -2L, Long.MIN_VALUE, ...). Such masks do not clear the upper 32 bits of a lane, so the resulting VPMULUDQ returned incorrect products. Fix: cast the constant to julong so the bound is evaluated unsigned.

Please review and share your feedback.

Best Regards,
Jatin

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

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

Issue

• JDK-8384963: C2: Incorrect uint constant match mishandles negative values in vectors (Bug - P4)

Reviewers

• Eric Fang (@erifan - Author)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 31305

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jbhateja! 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]

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

[openjdk]

@jatin-bhateja 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.

[openjdk]

The total number of required reviews for this PR has been set to 2 based on the presence of this label: hotspot-compiler. This can be overridden with the /reviewers command.

[mlbridge]

Webrevs

• 01: Full - Incremental (96c7ee9f)
• 00: Full (c54a0bd4)

[shipilev]

I raised this question in the JBS comments, not sure if that is a real problem or not?

"Actually, I think the similar trouble awaits with predicated AND in this case. If predicated mask passes the full 64-bit lane through AND without change, we cannot trust the matchers as well. We probably need to sprinkle is_predicated_vector() checks in these and test them thoroughly."

[erifan]

I think we need to disable predicated OPs, as @shipilev mentioned above.

[jatin-bhateja]

I raised this question in the JBS comments, not sure if that is a real problem or not?

"Actually, I think the similar trouble awaits with predicated AND in this case. If predicated mask passes the full 64-bit lane through AND without change, we cannot trust the matchers as well. We probably need to sprinkle is_predicated_vector() checks in these and test them thoroughly."

Hi @shipilev ,
I think pre matcher graph re-shaping https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/matcher.cpp#L2239
Will generate a graph shape for predicated vector IR where source operands (masking Replicates) will be pushed behind BinaryNode. This will bypass the existing pattern matches in MulVLNode::has_unit_inputs and MulVLNode::has_int_inputs. But we can still add predication checks to make is independent of graph reshaping

[shipilev]

Oh, all right, it looks that way. As long as you cannot construct a counter-example :)

[erifan]

@jatin-bhateja Your analysis is correct, but it relies on how find_shared_post_visit builds the binary tree for req()==4 predicated vectors (in(1)=Binary, in(2)=mask). If that reshape ever changes, has_vector_elements_fit_uint / has_vector_elements_fit_int can silently start accepting predicated AndV / URShiftVL / RShiftVL inputs and re-introduce exactly this class of miscompile.

I still suggest adding an explicit is_predicated_vector() check or a comment, because the safety here is an implicit cross-file invariant and the next person touching either side will not see it.

[jatin-bhateja]

@jatin-bhateja Your analysis is correct, but it relies on how find_shared_post_visit builds the binary tree for req()==4
I still suggest adding an explicit is_predicated_vector() check or a comment,

I think there is no harm in adding safety non-predication checks

[erifan]

Thanks~