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

8246603: [lworld] C2 does not scalarize inline types wrapped into non-escaping box objects #71

Closed
wants to merge 3 commits into from

Conversation

@TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented Jun 9, 2020

C2 fails to scalarize inline types wrapped into non-inline, non-escaping (box) objects.

For example, in TestLWorld::test109, C2 successfully scalar replaces the InterfaceBox object but not the LongWrapper object it contains because of the complex control flow in LongWrapper::wrap. However, since LongWrapper is an inline type, we don't need to rely on Escape Analysis to be able to scalar replace. Now the problem is that LongWrapper is stored as oop in a field of type WrapperInterface and we don't keep track of the ValueType(Ptr)Node long enough (i.e. until after EA) for the load to be removed and the buffer allocation to go away.

The fix contains the following changes:

  • Use ValueTypePtrNode instead of the oop whenever possible to keep track of field values. PhiNode::Ideal will then push such ValueTypePtrNode down and LoadNode::Identity will fold the loads.
  • Keep ValueTypePtrNodes such that we can still fold loads after EA removed potential non-inline, wrapper objects that prevented scalarization during parsing. Only remove them after EA is done.
  • Piggy-backing on PhaseMacroExpand::eliminate_allocate_node to eliminate unused inline type allocations and removed Allocate::Ideal which is not needed anymore (it also did not remove allocations that still had initializing stores).
  • Added code to remove the membar added after inline type allocation (it will otherwise block loop opts).
  • Make sure phis are always split if all inputs are mergemems to remove useless memory merges that block optimizations (see JDK-8247216)
  • Added regression tests and a benchmark (provided by Maurizio)

Performance without fix:

Benchmark Mode Cnt Score Error Units
TestBoxing.pojo_loop avgt 30 4.699 ± 0.045 ms/op
TestBoxing.box_generic_loop avgt 30 4.540 ± 0.058 ms/op
TestBoxing.box_inline_loop avgt 30 0.527 ± 0.009 ms/op
TestBoxing.box_intf_loop avgt 30 4.512 ± 0.050 ms/op
TestBoxing.box_ref_loop avgt 30 4.551 ± 0.037 ms/op
TestBoxing.inline_loop avgt 30 0.524 ± 0.013 ms/op

Performance with fix:

Benchmark Mode Cnt Score Error Units
TestBoxing.pojo_loop avgt 30 4.818 ± 0.166 ms/op
TestBoxing.box_generic_loop avgt 30 0.517 ± 0.007 ms/op
TestBoxing.box_inline_loop avgt 30 0.513 ± 0.007 ms/op
TestBoxing.box_intf_loop avgt 30 0.523 ± 0.024 ms/op
TestBoxing.box_ref_loop avgt 30 0.511 ± 0.010 ms/op
TestBoxing.inline_loop avgt 30 0.514 ± 0.012 ms/op


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8246603: [lworld] C2 does not scalarize inline types wrapped into non-escaping box objects

Download

$ git fetch https://git.openjdk.java.net/valhalla pull/71/head:pull/71
$ git checkout pull/71

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 9, 2020

👋 Welcome back thartmann! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Jun 9, 2020

@TobiHartmann This change now passes all automated pre-integration checks, type /integrate in a new comment to proceed. After integration, the commit message will be:

8246603: [lworld] C2 does not scalarize inline types wrapped into non-escaping box objects
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

There are currently no new commits on the lworld branch since the last update of the source branch of this PR. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you would like to avoid potential automatic rebasing, specify the current head hash when integrating, like this: /integrate eab0f506aa460a1476be35308254ddd40149bb4d.

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 9, 2020

Webrevs

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 11, 2020

Mailing list message from Roland Westrelin on valhalla-dev:

Webrev: https://webrevs.openjdk.java.net/valhalla/71/webrev.00

Looks good.

Roland.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 12, 2020

Mailing list message from Tobias Hartmann on valhalla-dev:

Thanks Roland!

Fixed another issue with loads from constant fields not being folded and added some more tests:
https://github.com//pull/71/commits/dd252b81b7a7a3b549215416e4b3ab75b101392f

Best regards,
Tobias

On 11.06.20 13:02, Roland Westrelin wrote:

Webrev: https://webrevs.openjdk.java.net/valhalla/71/webrev.00

Looks good.

Roland.

@TobiHartmann
Copy link
Member Author

@TobiHartmann TobiHartmann commented Jun 12, 2020

/integrate

@openjdk openjdk bot closed this Jun 12, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 12, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Jun 12, 2020

@TobiHartmann
Pushed as commit f187e9d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
1 participant