8346039: [BACKOUT] - [C1] LIR Operations with one input should be implemented as LIR_Op1

[dholmes-ora]

Revert "8345609: [C1] LIR Operations with one input should be implemented as LIR_Op1"

This reverts commit a21d21f.

Testing tiers 1-3 in progress

Thanks

---

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-8346039: [BACKOUT] - [C1] LIR Operations with one input should be implemented as LIR_Op1 (Sub-task - P4)

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)
• Martin Doerr (@TheRealMDoerr - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22690

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

@dholmes-ora 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:

8346039: [BACKOUT] - [C1] LIR Operations with one input should be implemented as LIR_Op1

Reviewed-by: kvn, mdoerr

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 2 new commits pushed to the master branch:

• ddf0461: 8345799: Update copyright year to 2024 for core-libs in files where it was missed
• cbab40b: 8345683: Remove special flags for files compiled for static libraries

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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.

[openjdk]

@dholmes-ora 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

• 00: Full (26a13eff)

[vnkozlov]

Good.

[dholmes-ora]

Thanks for the review @vnkozlov !

Just awaiting testing to complete.

[TheRealMDoerr]

Backout looks correct. I wonder on what kind of machine the issue showed up. Was it 32 bit? Tier 2 has passed on our machines.

[dholmes-ora]

@TheRealMDoerr Windows -x64 and Linux-x64. It is intermittent (I guess random register content might cause that).

Thanks for the review.

[dholmes-ora]

@TheRealMDoerr I've added some failing test info to JDK-8346038

[TheRealMDoerr]

It must be machine dependent. I guess UseAVX > 2 && !VM_Version::supports_avx512vl() is the case which is not hit by our machines. Could you confirm that?

[dholmes-ora]

Yes the failing runs have -XX:UseAVX=3 -XX:+UnlockDiagnosticVMOptions -XX:+UseKNLSetting

[TheRealMDoerr]

Thanks for confirming!
@vnkozlov: Regarding the redo: Do you think we can remove the separate case

jdk/src/hotspot/cpu/x86/c1_LIRAssembler_x86.cpp

Line 3802 in 64fad1c

if (UseAVX > 2 && !VM_Version::supports_avx512vl()) {

and fall back to xorps in C1? That would simplify the code a lot and avoid an extra temp register and the preloading with LIR_OprFact::floatConst(-0.0)?

[vnkozlov]

@TheRealMDoerr I don't know simple answer for your question.

These checks were added after failures during testing changes for JDK-821076 diffs

Here is review of these changes: https://mail.openjdk.org/pipermail/hotspot-compiler-dev/2018-September/030629.html

The issue could be that we should avoid mixing encoding modes for AVX instructions (when C1 and C2 compiled methods call each other).

May be we should wait when @sviswa7 is back from vacation and get her answer to your question.

If it is not about mixing mode we can accept small performance issues since C1 generated code will be replaced with C2 generated.

[dholmes-ora]

/integrate

[openjdk]

Going to push as commit ec219ae.
Since your change was applied there have been 4 commits pushed to the master branch:

• 05c5678: 8345959: Make JVM_IsStaticallyLinked JVM_LEAF
• 64fad1c: 8345797: Update copyright year to 2024 for client-libs in files where it was missed
• ddf0461: 8345799: Update copyright year to 2024 for core-libs in files where it was missed
• cbab40b: 8345683: Remove special flags for files compiled for static libraries

Your commit was automatically rebased without conflicts.

[openjdk]

@dholmes-ora Pushed as commit ec219ae.

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

[merykitty]

@TheRealMDoerr I have not looked too closely at your change, but from the error messages I guess the failure may be due to the fact that input operands are not required to live during the node, while temp operands live only during the node. As a result, they may be assigned the same register by the allocator, which is not correct for some of the nodes here.

[TheRealMDoerr]

Thanks everyone for looking into the issue and the assistance. I have understood the problem. C1 allows using the same register for a temp and an input operand. Another problem is that the UseKNLSetting code needs the floatConst(-0.0) for lir_abs and lir_neg.
Only the code for UseAVX > 2 && !VM_Version::supports_avx512vl() is affected. The rest looks correct and all tests had passed without this mode.
I'll check with @sviswa7 if we can remove this extra code from C1.

[TheRealMDoerr]

I've created a draft PR with the removal: #22709
The new code actually looks better to me, but I'll wait for the feedback from Intel.