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

8266746: C1: Replace UnsafeGetRaw with UnsafeGet when setting up OSR entry block #3917

Closed
wants to merge 5 commits into from

Conversation

@kelthuzadx
Copy link
Member

@kelthuzadx kelthuzadx commented May 7, 2021

After JDK-8150921, most Unsafe{Get,Put}Raw intrinsic methods can be replaced by Unsafe{Get,Put}Object.

There is the only one occurrence where c1 refers UnsafeGetRaw among GraphBuilder::setup_osr_entry_block()

for_each_local_value(state, index, local) {
int offset = locals_offset - (index + local->type()->size() - 1) * BytesPerWord;
Value get;
if (local->type()->is_object_kind() && !live_oops.at(index)) {
// The interpreter thinks this local is dead but the compiler
// doesn't so pretend that the interpreter passed in null.
get = append(new Constant(objectNull));
} else {
get = append(new UnsafeGetRaw(as_BasicType(local->type()), e,
append(new Constant(new IntConstant(offset))),
0,
true /*unaligned*/, true /*wide*/));
}
_state->store_local(index, get);
}

We can replace UnsafeGetRaw with UnsafeGetObject when setting up OSR entry block. After that, Unsafe{Get,Put}Raw can be completely removed because no one refers to them.

(This patch actually does two things:

  1. Replace UnsafeGetRaw with UnsafeGetObject when setting up OSR entry block This is the only occurrence where c1 refers UnsafeGetRaw
  2. Cleanup unused Unsafe{Get,Put}Raw code
    They are related so I put it together, but I still want to hear your suggestions, I will separate them into two patches if you think it is more reasonable)

Thanks!
Yang


Progress

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

Issue

  • JDK-8266746: C1: Replace UnsafeGetRaw with UnsafeGet when setting up OSR entry block

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3917

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 7, 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 May 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 7, 2021

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 7, 2021

Loading

@dean-long
Copy link
Member

@dean-long dean-long commented May 7, 2021

Hi Yang. If the OptimizeUnsafes feature was ever useful for UnsafeGetRaw, then maybe we should apply that optimization to the UnsafeGetObject-with-null-object case rather than removing the feature. Could you try it?

Loading

@dean-long
Copy link
Member

@dean-long dean-long commented May 7, 2021

Also, I can't find where the UnsafeGetObject implementation handles the unaligned/wide flags of UnsafeGetRaw.

It would be good to get this change reviewed by someone from GC, since UnsafeGetRaw uses the GC barrier interface to do its work.

Loading

@dean-long
Copy link
Member

@dean-long dean-long commented May 7, 2021

/cc hotspot-gc

Loading

@openjdk openjdk bot added the hotspot-gc label May 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 7, 2021

@dean-long
The hotspot-gc label was successfully added.

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented May 8, 2021

Hi Dean,

Thanks for noticing this PR.

If the OptimizeUnsafes feature was ever useful for UnsafeGetRaw, then maybe we should apply that optimization to the UnsafeGetObject-with-null-object case rather than removing the feature. Could you try it?

Yes, I will investigate this and file an issue if it's possible to canonicalize UnsafeGetObject as well.

Also, I can't find where the UnsafeGetObject implementation handles the unaligned/wide flags of UnsafeGetRaw.

I think access_load already handles the wide flag

__ move_wide(access.resolved_addr()->as_address_ptr(), result);

Internally, move_wide will check the type and decide whether to execute the "wide" semantics according to the type, so using access_load for all types is ok.

As for the unaligned flag, I found that it only affects PPC and s390 (which is why the pre-submit test passed), and access_load is not aware of this flag, I think they should really need to be handled.

It would be good to get this change reviewed by someone from GC, since UnsafeGetRaw uses the GC barrier interface to do its work.

Yes, I'm looking forward to hearing suggestions from GC folks!

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented May 19, 2021

PING: May I ask your help to review this patch?

Loading

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Looks good to me but this should be tested on all platforms.

Loading

LIR_Opr offset = new_register(T_LONG);
__ convert(Bytecodes::_i2l, off.result(), offset);
#else
LIR_Opr offset = off.result();
Copy link
Member

@TobiHartmann TobiHartmann May 19, 2021

Choose a reason for hiding this comment

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

The indentation is wrong in the else branch.

Loading

true /*unaligned*/, true /*wide*/));
Value off_val = append(new Constant(new IntConstant(offset)));
get = append(new UnsafeGetObject(as_BasicType(local->type()), e,
off_val,
Copy link
Member

@TobiHartmann TobiHartmann May 19, 2021

Choose a reason for hiding this comment

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

Indentation is wrong.

Loading

}

// accessors
bool is_raw_get() { return _is_raw_get; }
Copy link
Member

@TobiHartmann TobiHartmann May 19, 2021

Choose a reason for hiding this comment

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

I would rename this to _is_raw because we already know it's a get.

Loading

Copy link
Member Author

@kelthuzadx kelthuzadx May 20, 2021

Choose a reason for hiding this comment

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

Thanks Tobias for the review! All fixed. I will test it on Linux(already tested)/Mac/Windows(aarch64+x86_64) later. But I don't have ppc and s390 machines so I'm not sure how to test it on them...

Loading

Copy link
Member

@TobiHartmann TobiHartmann May 21, 2021

Choose a reason for hiding this comment

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

Looks good. Maybe someone from the OpenJDK community can run some sanity testing on ppc/s390.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 19, 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:

8266746: C1: Replace UnsafeGetRaw with UnsafeGet when setting up OSR entry block

Replace UnsafeGetRaw with UnsafeGetObject when setting up OSR entry block, and rename Unsafe{Get,Put}Object to Unsafe{Get,Put}

Reviewed-by: thartmann, dlong, 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 861 new commits pushed to the master branch:

  • a6b253d: 8269416: [JVMCI] capture libjvmci crash data to a file
  • a0f32cb: 8268906: gc/g1/mixedgc/TestOldGenCollectionUsage.java assumes that GCs take 1ms minimum
  • ee0247f: 8263461: jdk/jfr/event/gc/detailed/TestEvacuationFailedEvent.java uses wrong mechanism to cause evacuation failure
  • 3ad20fc: 8269571: NMT should print total malloc bytes and invocation count
  • b969136: 8245877: assert(_value != __null) failed: resolving NULL _value in JvmtiExport::post_compiled_method_load
  • ee526a2: Merge
  • 0d745ae: 8269034: AccessControlException for SunPKCS11 daemon threads
  • d042029: 8269529: javax/swing/reliability/HangDuringStaticInitialization.java fails in Windows debug build
  • 401cb0a: 8269232: assert(!is_jweak(handle)) failed: wrong method for detroying jweak
  • b8a16e9: 8268884: C2: Compile::remove_speculative_types must iterate top-down
  • ... and 851 more: https://git.openjdk.java.net/jdk/compare/80941f475f7f3bd479f1ab75287f0ffe7935ad05...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 19, 2021
Copy link
Member

@dean-long dean-long left a comment

It looks like when JDK-8150921 was done, some micro-benchmarks were run, and @shipilev noticed a regression in C1 with constant null handling:
http://cr.openjdk.java.net/~shade/8150921/notes.txt
Looking over the code review:
https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2016-May/022767.html
I don't see any references to benchmark testing that would check for any regressions in the InlineUnsafeOps + OptimizeUnsafes optimization that 8150921 mostly bypassed. @vidmik
Now this PR proposes to remove this mostly dead code. I'm OK with that, since blindly reviving it risks hitting bitrot, but for compleness, perhaps we should file 2 followup RFEs:

  1. investigate constant null C1 regression
  2. measure if the dead InlineUnsafeOps + OptimizeUnsafes optimization is worth reviving in a later release (with sufficient bake time)

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented May 26, 2021

It looks like when JDK-8150921 was done, some micro-benchmarks were run, and @shipilev noticed a regression in C1 with constant null handling:
http://cr.openjdk.java.net/~shade/8150921/notes.txt
Looking over the code review:
https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2016-May/022767.html
I don't see any references to benchmark testing that would check for any regressions in the InlineUnsafeOps + OptimizeUnsafes optimization that 8150921 mostly bypassed. @vidmik
Now this PR proposes to remove this mostly dead code. I'm OK with that, since blindly reviving it risks hitting bitrot, but for compleness, perhaps we should file 2 followup RFEs:

  1. investigate constant null C1 regression
  2. measure if the dead InlineUnsafeOps + OptimizeUnsafes optimization is worth reviving in a later release (with sufficient bake time)

They are indeed necessary. Thanks for sharing more information about JDK-8150921. I've filed JDK-8267783 and JDK-8267782 and assign them to me, I would investigate and check if it's worth doing later.

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Jun 1, 2021

Tests are passed on both aarch64 and x64. Can someone help running sanity checks on s390/ppc machines? Thanks a lot.

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Jun 25, 2021

@TobiHartmann @dean-long It seems that there is no easy way to test this PR on all architectures. Can I merge this PR in this case? If not, do you have a general solution for changes that should be tested on all architectures while the author can not cover all architectures for PR that they made? Thanks in advance!

Loading

@dean-long
Copy link
Member

@dean-long dean-long commented Jun 25, 2021

@TheRealMDoerr, could someone at SAP test this change on s390 and ppc, please?

Loading

@TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Jun 28, 2021

Thanks for checking with us! I've given it a quick spin and it seems to work on s390. However, I got the following error on ppc64:
Internal Error (src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp:1175)
assert(!unaligned) failed: unexpected

V [libjvm.so+0x64703c] LIR_Assembler::mem2reg(LIR_OprDesc*, LIR_OprDesc*, BasicType, LIR_PatchCode, CodeEmitInfo*, b
ool, bool)+0x51c
V [libjvm.so+0x6399e4] LIR_Assembler::move_op(LIR_OprDesc*, LIR_OprDesc*, BasicType, LIR_PatchCode, CodeEmitInfo*, b
ool, bool, bool)+0x464
V [libjvm.so+0x63abac] LIR_Assembler::emit_op1(LIR_Op1*)+0x56c
V [libjvm.so+0x62283c] LIR_Op1::emit_code(LIR_Assembler*)+0x2c
V [libjvm.so+0x63bddc] LIR_Assembler::emit_lir_list(LIR_List*)+0x11c

Loading

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

If you want to use unaligned_move, please remove the assertions (the derived PPC64 instructions do support unaligned access):

--- a/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp
+++ b/src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp
@@ -1172,7 +1172,6 @@ void LIR_Assembler::mem2reg(LIR_Opr src_opr, LIR_Opr dest, BasicType type,
     assert(Assembler::is_simm16(disp_value), "should have set this up");
     offset = load(src, disp_value, to_reg, type, wide, unaligned);
   } else {
-    assert(!unaligned, "unexpected");
     offset = load(src, disp_reg, to_reg, type, wide);
   }
 
@@ -1301,7 +1300,6 @@ void LIR_Assembler::reg2mem(LIR_Opr from_reg, LIR_Opr dest, BasicType type,
     assert(Assembler::is_simm16(disp_value), "should have set this up");
     offset = store(from_reg, src, disp_value, type, wide, unaligned);
   } else {
-    assert(!unaligned, "unexpected");
     offset = store(from_reg, src, disp_reg, type, wide);
   }
 

Loading

#endif
LIR_Address* addr = new LIR_Address(src.result(), offset, type);
if (type == T_LONG || type == T_DOUBLE) {
__ unaligned_move(addr, result);
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Jun 28, 2021

Choose a reason for hiding this comment

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

You're always using unaligned_move. Old code checks x->may_be_unaligned(). May be ok, though.

Loading

Copy link
Member Author

@kelthuzadx kelthuzadx Jun 29, 2021

Choose a reason for hiding this comment

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

Thank you Martin for running these tests!

In this case, may_be_unaligned is must-be-unaligned since setup_osr_entry_block always creates unaligned UnsafeGetRaw before.

Loading

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

Thanks for taking care of PPC64. I haven't seen errors with this version. But I have some remarks:

  • I think the name UnsafeGetObject is confusing, now. It's no longer used for objects only.
  • The "unaligned" parameter is unused on all platforms, now. Note that PPC64 and s390 support unaligned accesses, too. I believe it was used by SPARC which has been removed.

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Jun 30, 2021

Thanks for taking care of PPC64. I haven't seen errors with this version. But I have some remarks:

  • I think the name UnsafeGetObject is confusing, now. It's no longer used for objects only.
  • The "unaligned" parameter is unused on all platforms, now. Note that PPC64 and s390 support unaligned accesses, too. I believe it was used by SPARC which has been removed.
  1. Yes, I agree, how about changing Unsafe{Get,Put}Object to Unsafe{Get,Put}:
void GraphBuilder::build_graph_for_intrinsic(ciMethod* callee, bool ignore_return) {
  vmIntrinsics::ID id = callee->intrinsic_id();
  assert(id != vmIntrinsics::_none, "must be a VM intrinsic");

  // Some intrinsics need special IR nodes.
  switch(id) {
  case vmIntrinsics::_getReference       : append_unsafe_get_obj(callee, T_OBJECT,  false); return;
  case vmIntrinsics::_getBoolean         : append_unsafe_get_obj(callee, T_BOOLEAN, false); return;
  case vmIntrinsics::_getByte            : append_unsafe_get_obj(callee, T_BYTE,    false); return;
  case vmIntrinsics::_getShort           : append_unsafe_get_obj(callee, T_SHORT,   false); return;
  case vmIntrinsics::_getChar            : append_unsafe_get_obj(callee, T_CHAR,    false); return;
  case vmIntrinsics::_getInt             : append_unsafe_get_obj(callee, T_INT,     false); return;
  case vmIntrinsics::_getLong            : append_unsafe_get_obj(callee, T_LONG,    false); return;
  case vmIntrinsics::_getFloat           : append_unsafe_get_obj(callee, T_FLOAT,   false); return;
  
  ...
}


void GraphBuilder::append_unsafe_get_obj(ciMethod* callee, BasicType t, bool is_volatile) {
  Values* args = state()->pop_arguments(callee->arg_size());
  null_check(args->at(0));
  Instruction* offset = args->at(2);
#ifndef _LP64
  offset = append(new Convert(Bytecodes::_l2i, offset, as_ValueType(T_INT)));
#endif
  Instruction* op = append(new UnsafeGetObject(t, args->at(1), offset, is_volatile));
  push(op->type(), op);
  compilation()->set_has_unsafe_access(true);
}

I don't see any reason that we need the "Object" suffix.

  1. Maybe we can file a follow-up issue to investigate if it's indeed unnecessary now.

Loading

@TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Jun 30, 2021

  1. Unsafe{Get,Put} sound good to me. I'd appreciate the change and the removal of the _obj suffix.
  2. Sure. The "unaligned" cleanup was just a remark. It doesn't need to get addressed as part of this change.
    Thank you!

Loading

@kelthuzadx kelthuzadx changed the title 8266746: C1: Replace UnsafeGetRaw with UnsafeGetObject when setting up OSR entry block 8266746: C1: Replace UnsafeGetRaw with UnsafeGet when setting up OSR entry block Jun 30, 2021
@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Jun 30, 2021

/summary Replace UnsafeGetRaw with UnsafeGetObject when setting up OSR entry block, and rename Unsafe{Get,Put}Object to Unsafe{Get,Put}

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 30, 2021

@kelthuzadx Setting summary to Replace UnsafeGetRaw with UnsafeGetObject when setting up OSR entry block, and rename Unsafe{Get,Put}Object to Unsafe{Get,Put}

Loading

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

Thanks for cleaning this up!

Loading

@kelthuzadx
Copy link
Member Author

@kelthuzadx kelthuzadx commented Jul 1, 2021

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jul 1, 2021

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

  • 4660f72: 8268870: Remove dead code in metaspaceShared
  • 9def3b0: Merge
  • 9ac63a6: 8262841: Clarify the behavior of PhantomReference::refersTo
  • aba6c55: 8269703: ProblemList vmTestbase/nsk/jvmti/scenarios/sampling/SP07/sp07t002/TestDescription.java on Windows-X64 with -Xcomp
  • 3e02224: 8269513: Clarify the spec wrt useOldISOCodes system property
  • 0dc65d3: 8268897: [TESTBUG] compiler/compilercontrol/mixed/RandomCommandsTest.java must not fail on Command.quiet
  • 3826012: 8268557: Module page uses unstyled table class
  • 2b17e95: 8269691: ProblemList sun/management/jdp/JdpDefaultsTest.java on Linux-aarch64
  • 1da5d4b: 8269486: CallerAccessTest fails for non server variant
  • be0ac92: 8269614: [s390] Interpreter checks wrong bit for slow path instance allocation
  • ... and 874 more: https://git.openjdk.java.net/jdk/compare/80941f475f7f3bd479f1ab75287f0ffe7935ad05...master

Your commit was automatically rebased without conflicts.

Loading

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

@openjdk openjdk bot commented Jul 1, 2021

@kelthuzadx Pushed as commit d89e630.

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

Loading

@kelthuzadx kelthuzadx deleted the c1_cleanup branch Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants