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

8265322: C2: Simplify control inputs for BarrierSetC2::obj_allocate #3529

Closed

Conversation

@kelthuzadx
Copy link
Member

@kelthuzadx kelthuzadx commented Apr 16, 2021

While just using BarrierSetC2::obj_allocate in openjdk/valhalla#385, I noticed the control input ctrl for obj_allocate is not so much necessary. This PR simplifies control inputs for BarrierSetC2::obj_allocate. In most cases, it doesn't change anything since toobig_false is equivalent to ctrl. In other cases, toobig_false is created for Unsafe.allocateInstance while instance size is not statically known, ctrl would become control input of IfNode whose projects are toobig_false and toobig_true, old eden_end and old_eden_top can simply accept toobig_false as their control input rather than ctrl.


Progress

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

Issue

  • JDK-8265322: C2: Simplify control inputs for BarrierSetC2::obj_allocate

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3529

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 16, 2021

👋 Welcome back yyang! 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 openjdk bot added the rfr label Apr 16, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 16, 2021

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

  • hotspot

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

@openjdk openjdk bot added the hotspot label Apr 16, 2021
@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Apr 16, 2021

/label remove hotspot
/label add hotspot-compiler

Loading

@openjdk openjdk bot removed the hotspot label Apr 16, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 16, 2021

@kelthuzadx
The hotspot label was successfully removed.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 16, 2021

@kelthuzadx
The hotspot-compiler label was successfully added.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 16, 2021

Webrevs

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Apr 16, 2021

/label add hotspot-gc

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Apr 16, 2021

This should be reviewed by GC group who owns this code.

Loading

@openjdk openjdk bot added the hotspot-gc label Apr 16, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 16, 2021

@vnkozlov
The hotspot-gc label was successfully added.

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

From compiler code POV the fix is reasonable and correct.

Note, the path when initial_slow_test != NULL is not rare. It is frequent for arrays allocation when length is not constant.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 16, 2021

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

8265322: C2: Simplify control inputs for BarrierSetC2::obj_allocate

Reviewed-by: kvn, neliasso

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

  • dd8286e: 8198616: java/awt/Focus/6378278/InputVerifierTest.java fails on mac
  • 5574922: 8266284: ProblemList java/awt/Graphics2D/DrawString/DrawRotatedStringUsingRotatedFont.java
  • df7f0b4: 8198317: Enhance JavacTool.getTask for flexibility
  • 115a413: 8265123: Add static factory methods to com.sun.net.httpserver.Filter
  • 39abac9: 8266176: -Wmaybe-uninitialized happens in libArrayIndexOutOfBoundsExceptionTest.c
  • 155da25: 8265005: Introduce the new client property for mac: apple.awt.windowTitleVisible
  • 91226fa: 8265940: Enable C2's optimization for Math.pow(x, 0.5) on all platforms
  • 56cde70: 8266265: mark hotspot compiler/vectorization tests which ignore VM flags
  • 4937214: 8266174: -Wmisleading-indentation happens in libmlib_image sources
  • b305eff: 8266238: mark hotspot compiler/inlining tests which ignore VM flags
  • ... and 331 more: https://git.openjdk.java.net/jdk/compare/ecef1fc82b71addff4345f98f53946525c000067...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@vnkozlov, @neliasso) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Loading

@openjdk openjdk bot added the ready label Apr 16, 2021
@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Apr 19, 2021

From compiler code POV the fix is reasonable and correct.

Thank you @vnkozlov!

Do you think it's reasonable to move PhaseMacroExpand::set_eden_pointers to BarrierSetC2? It seems that that's GC knowledge area about how to set eden_top/eden_end w or w/o turning UseTLAB.

Note, the path when initial_slow_test != NULL is not rare. It is frequent for arrays allocation when length is not constant.

Yes, I missed the AllocateArrayNode when skimming the code. Thanks for pointing out this.

Let's wait for other hotspot gc/compiler folks for more reviews.

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Apr 19, 2021

From compiler code POV the fix is reasonable and correct.
Do you think it's reasonable to move PhaseMacroExpand::set_eden_pointers to BarrierSetC2? It seems that that's GC knowledge area about how to set eden_top/eden_end w or w/o turning UseTLAB.

It is up to GC group but it is common code for all GCs. As I understand BarrierSetC2 is used for GC variations code.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 27, 2021

Mailing list message from Yi Yang on hotspot-gc-dev:

Gentle Ping :-
Can I have a second review of this patch?

Thanks,Yang

------------------------------------------------------------------
From:Yi Yang <yyang at openjdk.java.net>
Send Time:2021 Apr. 16 (Fri.) 11:16
To:hotspot-compiler-dev <hotspot-compiler-dev at openjdk.java.net>
Subject:RFR: 8265322: C2: Simplify control inputs for BarrierSetC2::obj_allocate

This PR simplifies control inputs for BarrierSetC2::obj_allocate. In most cases, it doesn't change anything since `toobig_false` is equivalent to `ctrl`. In rare case, `toobig_false` is created for Unsafe.allocateInstance while instance size is not statically known, `ctrl` would become control input of IfNode whose projects are `toobig_false` and `toobig_true`, old eden_end and old_eden_top can simply accept `toobig_false` as their control input rather than `ctrl`.

-------------

Commit messages:
- simplify

Changes: https://git.openjdk.java.net/jdk/pull/3529/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3529&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8265322
Stats: 5 lines in 3 files changed: 0 ins; 0 del; 5 mod
Patch: https://git.openjdk.java.net/jdk/pull/3529.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3529/head:pull/3529

PR: https://git.openjdk.java.net/jdk/pull/3529

Loading

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 27, 2021

Mailing list message from Yi Yang on hotspot-gc-dev:

Gentle Ping :-
Can I have a second review of this patch?

Thanks,Yang

------------------------------------------------------------------
From:Yi Yang <yyang at openjdk.java.net>
Send Time:2021 Apr. 16 (Fri.) 11:16
To:hotspot-compiler-dev <hotspot-compiler-dev at openjdk.java.net>
Subject:RFR: 8265322: C2: Simplify control inputs for BarrierSetC2::obj_allocate

This PR simplifies control inputs for BarrierSetC2::obj_allocate. In most cases, it doesn't change anything since `toobig_false` is equivalent to `ctrl`. In rare case, `toobig_false` is created for Unsafe.allocateInstance while instance size is not statically known, `ctrl` would become control input of IfNode whose projects are `toobig_false` and `toobig_true`, old eden_end and old_eden_top can simply accept `toobig_false` as their control input rather than `ctrl`.

-------------

Commit messages:
- simplify

Changes: https://git.openjdk.java.net/jdk/pull/3529/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3529&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8265322
Stats: 5 lines in 3 files changed: 0 ins; 0 del; 5 mod
Patch: https://git.openjdk.java.net/jdk/pull/3529.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/3529/head:pull/3529

PR: https://git.openjdk.java.net/jdk/pull/3529

Loading

Copy link
Contributor

@neliasso neliasso left a comment

Looks good.

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Apr 29, 2021

Thanks @vnkozlov and @neliasso for reviews!

/integrate

Loading

@openjdk openjdk bot added the sponsor label Apr 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 29, 2021

@kelthuzadx
Your change (at version a17a2b6) is now ready to be sponsored by a Committer.

Loading

@neliasso
Copy link
Contributor

@neliasso neliasso commented May 3, 2021

/sponsor

Loading

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

@openjdk openjdk bot commented May 3, 2021

@neliasso @kelthuzadx Since your change was applied there have been 386 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 376 more: https://git.openjdk.java.net/jdk/compare/ecef1fc82b71addff4345f98f53946525c000067...master

Your commit was automatically rebased without conflicts.

Pushed as commit 001c514.

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

Loading

@kelthuzadx kelthuzadx deleted the obj_allocate_handle_toobig branch May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants