8322816: RISC-V: Incorrect guarantee in patch_vtype

[kimbarrett]

Please review this change that fixes a test for a guarantee. This also
removes a -Wparentheses warning when those are enabled (which is how the
problem was discovered).

The problem is that operator precedence groups the sub-expressions differently
than intended. The fix is to override the operator precedence by adding
parentheses to achieve the intended grouping.

Testing: Local (linux-x64) cross-build for linux-riscv with this change plus
-Wparentheses enabled and other changes to allow that to work.

Requesting someone from the riscv porters to properly test this.

---

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-8322816: RISC-V: Incorrect guarantee in patch_vtype (Bug - P4)

Reviewers

• Fei Yang (@RealFYang - Reviewer)
• Ludovic Henry (@luhenry - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17215

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

@kimbarrett 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]

Webrevs

• 02: Full - Incremental (22fc7a2b)
• 01: Full - Incremental (ab335602)
• 00: Full (a3723801)

[RealFYang]

I see the vill parameter is always false in current code, which means this guarantee never gets excecuted. And I don't think we would make use of the vill field of vtype in future. So I personally perfer to remove this guarantee and its enclosing if block for now.

[kimbarrett]

Rather than removing the guarantee, wouldn't it be better to guarantee/assert vill == 0?
Although looking at uses, that argument is a bool, so it should be guarantee(!vill, ...).

[RealFYang]

Hi, Yes, that's better. Maybe: guarantee(!vill, "should be");

[kimbarrett]

I've changed the guarantee as discussed. There are further cleanups possible
here, but I'll leave that to the riscv port maintainers.

[openjdk]

@kimbarrett This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8322816: RISC-V: Incorrect guarantee in patch_vtype

Reviewed-by: fyang, luhenry

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[kimbarrett]

Waiting for second review.

[kimbarrett]

Thanks for reviews, @RealFYang and @luhenry .

[kimbarrett]

/integrate

[openjdk]

Going to push as commit f4ca41a.

[openjdk]

@kimbarrett Pushed as commit f4ca41a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.