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

Jump to bottom

8264897: [lworld] C2: Use BarrierSetC2::obj_allocate to buffer inline type in PhaseMacroExpand::expand_mh_intrinsic_return #385

Closed
wants to merge 4 commits into from
Closed

8264897: [lworld] C2: Use BarrierSetC2::obj_allocate to buffer inline type in PhaseMacroExpand::expand_mh_intrinsic_return #385

wants to merge 4 commits into from

Conversation

kelthuzadx
Copy link
Member

@kelthuzadx kelthuzadx commented Apr 15, 2021 •
edited by openjdk[bot] bot

Try to reuse existing BarrierSetC2::obj_allocate functionality for PhaseMacroExpand::expand_mh_intrinsic_return.

Testing: x86_64 + linux+ slowdebug/release
[x] test/hotspot/jtreg/runtime/valhalla
[x] test/hotspot/jtreg/compiler/valhalla

Thanks,
Yang

Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8264897: [lworld] C2: Use BarrierSetC2::obj_allocate to buffer inline type in PhaseMacroExpand::expand_mh_intrinsic_return

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 385

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 15, 2021

👋 Welcome back yyang! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Apr 15, 2021 •
edited

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

8264897: [lworld] C2: Use BarrierSetC2::obj_allocate to buffer inline type in PhaseMacroExpand::expand_mh_intrinsic_return

Reviewed-by: thartmann

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

  • 301de8d: 8265184: [lworld] [AArch64] Broken fast class init barrier for static calls in c2i adapter
  • 2e6c276: 8264414: [lworld] [AArch64] TestBufferTearing.java fails with C1
  • e9c78ce: 8265118: [lworld] C1 should optimize inline type checkcasts
  • 4f88959: 8264895: [lworld] assert(!InstanceKlass::cast(receiver_klass)->is_not_initialized()) failed: receiver_klass must be initialized
  • 571322e: 8264977: [lworld] A primitive class field by name val confuses javac
  • f3950ad: 8264978: [lworld] Various small code cleanups
  • c68ce89: 8244227: [lworld] Explore an implementation where the reference projection and value projection types are backed by a single class symbol

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld 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.

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 (@TobiHartmann) 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).

@kelthuzadx kelthuzadx changed the title 8264897: [lworld] C2: Use arrierSetC2::obj_allocate to buffer inline type in PhaseMacroExpand::expand_mh_intrinsic_return 8264897: [lworld] C2: Use BarrierSetC2::obj_allocate to buffer inline type in PhaseMacroExpand::expand_mh_intrinsic_return Apr 15, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 15, 2021 •
edited

Webrevs

TobiHartmann
TobiHartmann approved these changes Apr 16, 2021
View changes
Copy link
Member

@TobiHartmann TobiHartmann left a comment

Nice cleanup! I've added some minor comments, otherwise looks good to me.

src/hotspot/share/opto/macro.cpp
break;
case Node::Class_FlatArrayCheck:
expand_flatarraycheck_node(n->as_FlatArrayCheck());
assert(C->macro_count() == (old_macro_count - 1), "expansion must have deleted one node from macro list");
Copy link
Member

@TobiHartmann TobiHartmann Apr 16, 2021

Choose a reason for hiding this comment

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

Why did you delete these asserts?

Copy link
Member Author

@kelthuzadx kelthuzadx Apr 16, 2021

Choose a reason for hiding this comment

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

It seems the checking happens nevertheless right after switch:

valhalla/src/hotspot/share/opto/macro.cpp

Lines 3220 to 3227 in 5e88e8a

break;
case Node::Class_FlatArrayCheck:
expand_flatarraycheck_node(n->as_FlatArrayCheck());
break;
default:
assert(false, "unknown node type in macro list");
}
assert(C->macro_count() == (old_macro_count - 1), "expansion must have deleted one node from macro list");

Copy link
Member

@TobiHartmann TobiHartmann Apr 20, 2021

Choose a reason for hiding this comment

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

Right, good catch!

src/hotspot/share/opto/macro.cpp Outdated
old_top = new LoadPNode(ctl, mem, top_adr, TypeRawPtr::BOTTOM, TypeRawPtr::BOTTOM, MemNode::unordered);
transform_later(old_top);
// Try to allocate a new buffered inline instance either from TLAB or eden space
Node* needgc_ctrl; // needgc means slowcase, i.e. allocation failed
Copy link
Member

@TobiHartmann TobiHartmann Apr 16, 2021

Choose a reason for hiding this comment

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

Should be initialized to NULL.

src/hotspot/share/opto/macro.cpp Outdated

Node* r = new RegionNode(4);
Node* r = new RegionNode(alloc_in_place ? 4: 3);
Copy link
Member

@TobiHartmann TobiHartmann Apr 16, 2021

Choose a reason for hiding this comment

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

Whitespace missing, should be ? 4 : 3

src/hotspot/share/opto/macro.cpp Outdated
transform_later(slowpath_false_mem);
Node* fast_ctl;
Node* fast_res;
MergeMemNode* fast_mem;
Copy link
Member

@TobiHartmann TobiHartmann Apr 16, 2021

Choose a reason for hiding this comment

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

Should be initialized to NULL.

src/hotspot/share/opto/macro.cpp Outdated
// We don't know how many values are returned. This assumes the
// worst case, that all available registers are used.
for (uint i = TypeFunc::Parms+1; i < domain->cnt(); i++) {
if (domain->field_at(i) == Type::HALF) {
slow_call->init_req(i, top());
handler_call->init_req(i+1, top());
if (alloc_in_place) handler_call->init_req(i+1, top());
Copy link
Member

@TobiHartmann TobiHartmann Apr 16, 2021

Choose a reason for hiding this comment

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

Please use parentheses for the if body. Same below.

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Apr 16, 2021

Thank you Tobias! Sorry for so many nits, I will check them more carefully in later PRs.

Best Regards,
Yang

@kelthuzadx kelthuzadx mentioned this pull request Apr 19, 2021
Closed
3 tasks
@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Apr 19, 2021

/integrate

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

@openjdk openjdk bot commented Apr 19, 2021

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

@TobiHartmann
Copy link
Member

@TobiHartmann TobiHartmann commented Apr 20, 2021

/sponsor

@openjdk openjdk bot closed this Apr 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 20, 2021

@TobiHartmann @kelthuzadx Since your change was applied there have been 7 commits pushed to the lworld branch:

  • 301de8d: 8265184: [lworld] [AArch64] Broken fast class init barrier for static calls in c2i adapter
  • 2e6c276: 8264414: [lworld] [AArch64] TestBufferTearing.java fails with C1
  • e9c78ce: 8265118: [lworld] C1 should optimize inline type checkcasts
  • 4f88959: 8264895: [lworld] assert(!InstanceKlass::cast(receiver_klass)->is_not_initialized()) failed: receiver_klass must be initialized
  • 571322e: 8264977: [lworld] A primitive class field by name val confuses javac
  • f3950ad: 8264978: [lworld] Various small code cleanups
  • c68ce89: 8244227: [lworld] Explore an implementation where the reference projection and value projection types are backed by a single class symbol

Your commit was automatically rebased without conflicts.

Pushed as commit feedec1.

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

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

@TobiHartmann TobiHartmann

Assignees
No one assigned
Labels
integrated
Milestone
No milestone
2 participants
@kelthuzadx @TobiHartmann