Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8265784: [C2] Hoisting of DecodeN leaves MachTemp inputs behind #3637

Closed

Conversation

@TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Apr 22, 2021

PPC64 and s390 have DecodeN implementations which use a MachTemp input. When LCM hoists the DecodeN, the MachTemp nodes reside in the old block, but should get hoisted together with the DecodeN node.
Same is true for load Base input which exists on s390 for example. Unfortunately, that's just a platform specific MachNode which is not nicely recognizable in LCM.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8265784: [C2] Hoisting of DecodeN leaves MachTemp inputs behind

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3637/head:pull/3637
$ git checkout pull/3637

Update a local copy of the PR:
$ git checkout pull/3637
$ git pull https://git.openjdk.java.net/jdk pull/3637/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3637

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3637.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 22, 2021

👋 Welcome back mdoerr! 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 22, 2021

@TheRealMDoerr 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.

Loading

@GoeLin
Copy link
Member

@GoeLin GoeLin commented Apr 22, 2021

Hi Martin,
I think the loop must go before the block->add_inst(val); .
Else you get a cycle within the block.
The MachTemp must be before the node that uses them in the list.
Best regards,
Goetz.

Loading

@TheRealMDoerr TheRealMDoerr marked this pull request as ready for review Apr 30, 2021
@openjdk openjdk bot added the rfr label Apr 30, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 30, 2021

Webrevs

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Apr 30, 2021

Moving additional TEMP and CONST? input edges to the same block is correct.
Is base always constant (like CompressedOops::base())? Otherwise you may need add anti dependences for load.
Can you show where MachTemp edges are added for DecodeN?

Loading

@TheRealMDoerr
Copy link
Contributor Author

@TheRealMDoerr TheRealMDoerr commented May 1, 2021

Hi Vladimir, thanks for reviewing my PR.
Yes, base is an immediate constant (CompressedOops::base()). It's not a load from memory.
MachTemp nodes are added and connected by ADLC generated code (ArchDesc::defineExpand). PPC64 and s390 code contains "expand" and "postalloc_expand" rules. E.g. decodeN_Ex in ppc64.ad uses a register with TEMP effect.

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Good.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 1, 2021

@TheRealMDoerr 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:

8265784: [C2] Hoisting of DecodeN leaves MachTemp inputs behind

Reviewed-by: kvn, goetz

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

  • 194bcec: 8265984: Concurrent GC: Some tests fail "assert(is_frame_safe(f)) failed: Frame must be safe"
  • 1d9ea3a: 8266083: Shenandoah: Consolidate dedup/no dedup oop closures
  • 80941f4: 8234446: Post-CMS workgroup hierarchy cleanup
  • ac760c7: 8266295: Remove unused _concurrent_iteration_safe_limit
  • b42d496: 8266388: C2: Improve constant ShiftCntV on x86
  • 05cfac9: 8266412: Remove redundant TemplateInterpreter entries
  • c5dc657: 8266056: runtime/stringtable/StringTableCleaningTest.java failed with "RuntimeException: Missing Callback in [10, 11]"
  • 8fa50eb: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction
  • 7e30130: 8266401: mark hotspot compiler/intrinsics/sha/cli tests which ignore VM flags
  • dedddd5: 8266248: Compilation failure in PLATFORM_API_MacOSX_MidiUtils.c with Xcode 12.5
  • ... and 177 more: https://git.openjdk.java.net/jdk/compare/b930bb1a2cb56fc9642f07e9688b6f358bfd1532...master

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.

Loading

@openjdk openjdk bot added the ready label May 1, 2021
Copy link
Member

@GoeLin GoeLin left a comment

Hi,
I think temp must go into a block that dominates both, tempb and block.
Do you know block always dominates tempb (after the if?)
Then you could add assert(block->dominates(tempb), "find legal posittion")
Best regards,
Goetz.

Loading

GoeLin
Copy link
Member

GoeLin commented on 0c893cd May 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good

Loading

GoeLin
GoeLin approved these changes May 3, 2021
Copy link
Member

@GoeLin GoeLin left a comment

LGTM

Loading

@TheRealMDoerr
Copy link
Contributor Author

@TheRealMDoerr TheRealMDoerr commented May 3, 2021

Hi Götz, thanks for reviewing!
Yes, block must always dominate tempb after the if. Otherwise, the placement would be illegal.
It's known that val (DecodeNNode) is movable to block at this place. So its inputs must be live along the same path.
I've added a sanity check.

Loading

@TheRealMDoerr
Copy link
Contributor Author

@TheRealMDoerr TheRealMDoerr commented May 3, 2021

Thanks for the reviews! I'll integrate it tomorrow if everything's fine.

Loading

@TheRealMDoerr
Copy link
Contributor Author

@TheRealMDoerr TheRealMDoerr commented May 4, 2021

/integrate

Loading

@openjdk openjdk bot closed this May 4, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 4, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 4, 2021

@TheRealMDoerr Since your change was applied there have been 197 commits pushed to the master branch:

  • ce1bc9d: 8266432: ZGC: GC allocation stalls can trigger deadlocks
  • 30ccd80: 8264950: Set opaque for JTooltip in config file of NimbusLookAndFeel
  • cfdf4a7: 8266449: cleanup jtreg tags in compiler/intrinsics/sha/cli tests
  • 3544a9d: 8266391: Replace use of reflection in jdk.internal.platform.Metrics
  • 020236c: 8264786: [macos] All Swing/AWT apps cause Allow Notifications prompt to appear when app is launched
  • 45760d4: 8266320: (bf) ReadOnlyBufferException in heap buffer put(String,int,int) should not be conditional
  • ff65920: 8265491: Math Signum optimization for x86
  • 55cc0af: 8266185: Shenandoah: Fix incorrect comment/assertion messages
  • 880c138: 8265349: vmTestbase/../stress/compiler/deoptimize/Test.java fails with OOME due to CodeCache exhaustion.
  • 001c514: 8265322: C2: Simplify control inputs for BarrierSetC2::obj_allocate
  • ... and 187 more: https://git.openjdk.java.net/jdk/compare/b930bb1a2cb56fc9642f07e9688b6f358bfd1532...master

Your commit was automatically rebased without conflicts.

Pushed as commit 8e071c4.

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

Loading

@TheRealMDoerr TheRealMDoerr deleted the 8265784_C2_DecodeN branch May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants