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

8260473: [vector] ZGC: VectorReshape test produces incorrect results with ZGC enabled #2253

Closed
wants to merge 7 commits into from

Conversation

@casparcwang
Copy link
Contributor

@casparcwang casparcwang commented Jan 27, 2021

https://bugs.openjdk.java.net/browse/JDK-8260473

Function "PhaseVector::expand_vunbox_node" creates a LoadNode, but forgets to make the LoadNode to pass gc barriers.

Testing: all Vector API related tests have passed.


Progress

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

Issue

  • JDK-8260473: [vector] ZGC: VectorReshape test produces incorrect results with ZGC enabled

Contributors

  • Stuart Monteith <smonteith@openjdk.org>
  • Wang Chao <casparcwang@tencent.com>

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2253/head:pull/2253
$ git checkout pull/2253

@bridgekeeper bridgekeeper bot added the oca label Jan 27, 2021
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 27, 2021

Hi @casparcwang, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user casparcwang" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 27, 2021

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

@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Jan 27, 2021

/signed

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 27, 2021

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Jan 27, 2021

/covered

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 27, 2021

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 28, 2021

@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Jan 28, 2021

/cc hotspot-gc

@openjdk openjdk bot added the hotspot-gc label Jan 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 28, 2021

@casparcwang
The hotspot-gc label was successfully added.

Copy link
Contributor

@neliasso neliasso left a comment

The patch looks good.

Please turn the reproducer into a regression test for this bug. Contact me if you need any help with that.

Copy link

@iwanowww iwanowww left a comment

What about migrating it to GraphKit::access_load_at instead?

@neliasso
Copy link
Contributor

@neliasso neliasso commented Jan 28, 2021

@iwanowww GraphKit::access_load_at is for parse time only. C2OptAccess must be used here.

@openjdk openjdk bot removed the rfr label Jan 28, 2021
@openjdk openjdk bot added the rfr label Jan 28, 2021
@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Jan 28, 2021

The test is part of test/jdk/jdk/incubator/vector/VectorReshapeTests.java.
And run the original big test with zgc didn't reproduce the failure, so just add a new small test.

The small test is provided by Stuart Monteith in the JBS. Thanks for providing the test.

/contributor add @stooart-mon

@openjdk
Copy link

@openjdk openjdk bot commented Jan 28, 2021

@casparcwang
Contributor Stuart Monteith <smonteith@openjdk.org> successfully added.

@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Jan 28, 2021

/contributor add casparcwang casparcwang@tencent.com

@openjdk
Copy link

@openjdk openjdk bot commented Jan 28, 2021

@casparcwang Could not parse @casparcwang as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Jan 28, 2021

I suggest adding the test under test/hotspot/jtreg/compiler/vectorapi.
Thanks.

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Jan 28, 2021

Since 32-bit VM doesn't support ZGC, the test should only for 64-bit.

@iwanowww
Copy link

@iwanowww iwanowww commented Jan 28, 2021

@iwanowww GraphKit::access_load_at is for parse time only. C2OptAccess must be used here.

I see. That's unfortunate.

Actually, PhaseVector::optimize_vector_boxes() sets C->inlining_incrementally() == true and it enables the code to use GraphKit and, moreover, perform late inlining of vector reboxing operations. But I haven't thought through all the implications yet.

@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Jan 28, 2021

@iwanowww GraphKit::access_load_at is for parse time only. C2OptAccess must be used here.

I see. That's unfortunate.

Actually, PhaseVector::optimize_vector_boxes() sets C->inlining_incrementally() == true and it enables the code to use GraphKit and, moreover, perform late inlining of vector reboxing operations. But I haven't thought through all the implications yet.

ArrayCopyNode::load performs the same work as it does here in PhaseVector::optimize_vector_boxes .
Is there a need to provide a similar function in PhaseVector or GraphKit?

@fisk
Copy link
Contributor

@fisk fisk commented Jan 28, 2021

Worth mentioning is that we have optimization-time loads and stores in the arraycopy/clone code. When that was added, it was the only place where we had such accesses, so a little load/store wrapper utility was written in the arraycopy code. But perhaps now that there is more than one place, that utility belongs in a more central place, so the boilerplate can be reduced. I'm okay with doing that in a follow-up RFE though, as it is a refactoring only, and getting the actual bug fix in soon is is of value.

/*
* @test
* @bug 8260473
* @modules jdk.incubator.vector
Copy link
Contributor

@pliden pliden Jan 28, 2021

This test should have a @requires vm.gc.Z tag. That will make sure it's only executed on platforms where ZGC is supported (and that ZGC is included in the build).

Copy link
Contributor Author

@casparcwang casparcwang Jan 29, 2021

done

Copy link
Contributor

@stooart-mon stooart-mon left a comment

Looks good, but needs some editing.

System.out.println("expect: "+Arrays.toString(expected));
System.out.println("output: "+Arrays.toString(output));
// Assert.assertEquals(expected, output);
assert(expected.equals(output)); // SRDM
Copy link
Contributor

@stooart-mon stooart-mon Jan 28, 2021

"SRDM" are my initials. You can remove this line and replace it with the uncommented line above.
I was structuring this to work outwith the jtreg framework.

Copy link
Contributor Author

@casparcwang casparcwang Jan 29, 2021

done

* -XX:-TieredCompilation -XX:CICompilerCount=1 -XX:+UseZGC -Xbatch -Xmx256m VectorReshapeTest
*/

public class VectorReshapeTest {
Copy link
Contributor

@stooart-mon stooart-mon Jan 28, 2021

This simply has this name as it is a cut-down version of test/jdk/jdk/incubator/vector/VectorReshapeTests.java
The problem was originally intermittent, but was narrowed somewhat down to what we have here. Perhaps this could be renamed?

Copy link
Contributor Author

@casparcwang casparcwang Jan 29, 2021

The test has changed to VectorRebracket128Test.java

* @bug 8260473
* @modules jdk.incubator.vector
* @modules java.base/jdk.internal.vm.annotation
* @run main/othervm -XX:CompileCommand=compileonly,jdk/incubator/vector/ByteVector.fromByteBuffer
Copy link
Contributor

@stooart-mon stooart-mon Jan 28, 2021

-XX:CompileCommand=compileonly,jdk/incubator/vector/ByteVector.fromByteBuffer restricts the compilation to a single method for diagnostic purposes. The test runs much quicker without it, and still reproduces the issue.

Copy link
Contributor Author

@casparcwang casparcwang Jan 29, 2021

The test is changed to 'testng' mode, remove option compileonly will make the test pass the assert in jtreg test framework. But add the option will make it fail the assert. So the option is left unchanged.

Copy link
Contributor

@stooart-mon stooart-mon Jan 29, 2021

I accept that - running it away from standalone does change the behaviour sufficiently that the CompileCommand is necessary to run the test (the standalone test was developed without the CompileCommand being necessary).

*/

public class VectorReshapeTest {
static final int INVOC_COUNT = Integer.getInteger("jdk.incubator.vector.test.loop-iterations", 100);
Copy link
Contributor

@stooart-mon stooart-mon Jan 28, 2021

The name "jdk.incubator.vector.test.loop-iterations" should probably be "jtreg.compiler.vectorapi.vectorreshapetest.loop-iterations".
In addition, it should be reset to "1000" to ensure the test is compiled and executed with a chance of GCing to occur.

Copy link
Contributor Author

@casparcwang casparcwang Jan 29, 2021

done

@iwanowww
Copy link

@iwanowww iwanowww commented Jan 28, 2021

ArrayCopyNode::load performs the same work as it does here in PhaseVector::optimize_vector_boxes .
Is there a need to provide a similar function in PhaseVector or GraphKit?

My point is since PhaseVector effectively enters the parsing phase (by signaling about the possibility of post-parse inlining), technically I don't see why GraphKit::access_load_at won't work. But I need to spend more time looking into the details.

So far, I took a look at the review thread of 8212243 (which introduced ArrayCopyNode::load) and found the following discussion between Roland and Erik:
https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2018-October/030971.html

> ... Also it beats  me that this is strictly speaking a load barrier for loads performed in 
> arraycopy. Would it be possible to use something like access_load_at  instead? ...
...
GraphKit is a parse time only thing. So the existing gc interface
doesn't offer any way to add barriers once parsing is over. This code
runs after parsing in optimization phases.
...

Considering PhaseVector::optimize_vector_boxes() already has access to a usable GraphKit instance, it is possible that GraphKit::access_load_at will "just work".

casparcwang added 2 commits Jan 29, 2021
1, require Z gc
2, Use testng Assert
3, Rename the test
4, get the right INVOC_COUNT
@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Jan 29, 2021

/contributor add Wang Chao casparcwang@tencent.com

@openjdk
Copy link

@openjdk openjdk bot commented Jan 29, 2021

@casparcwang
Contributor Wang Chao <casparcwang@tencent.com> successfully added.

@@ -0,0 +1,132 @@
/*
* Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights reserved.
Copy link
Contributor

@XiaohongGong XiaohongGong Jan 29, 2021

The Copyright should be " Copyright (c) 2021," since it's a new file added in 2021.

Copy link
Contributor Author

@casparcwang casparcwang Jan 29, 2021

The Copyright should be " Copyright (c) 2021," since it's a new file added in 2021.

done

@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Jan 29, 2021

ArrayCopyNode::load performs the same work as it does here in PhaseVector::optimize_vector_boxes .
Is there a need to provide a similar function in PhaseVector or GraphKit?

My point is since PhaseVector effectively enters the parsing phase (by signaling about the possibility of post-parse inlining), technically I don't see why GraphKit::access_load_at won't work. But I need to spend more time looking into the details.

So far, I took a look at the review thread of 8212243 (which introduced ArrayCopyNode::load) and found the following discussion between Roland and Erik:
https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2018-October/030971.html

> ... Also it beats  me that this is strictly speaking a load barrier for loads performed in 
> arraycopy. Would it be possible to use something like access_load_at  instead? ...
...
GraphKit is a parse time only thing. So the existing gc interface
doesn't offer any way to add barriers once parsing is over. This code
runs after parsing in optimization phases.
...

Considering PhaseVector::optimize_vector_boxes() already has access to a usable GraphKit instance, it is possible that GraphKit::access_load_at will "just work".

kit.access_load_at can be called here and make the test pass, but will lead to another test(test/hotspot/jtreg/compiler/vectorapi/VectorReinterpretTest.java) fail, I'm trying to find out why.

diff --git a/src/hotspot/share/opto/vector.cpp b/src/hotspot/share/opto/vector.cpp
index 671083e..69c00c5 100644
--- a/src/hotspot/share/opto/vector.cpp
+++ b/src/hotspot/share/opto/vector.cpp
@@ -414,17 +414,12 @@ void PhaseVector::expand_vunbox_node(VectorUnboxNode* vec_unbox) {
 
     Node* mem = vec_unbox->mem();
     Node* ctrl = vec_unbox->in(0);
-    Node* vec_field_ld;
-    {
-      DecoratorSet decorators = C2_READ_ACCESS | C2_CONTROL_DEPENDENT_LOAD | IN_HEAP;
-      C2AccessValuePtr addr(vec_adr, vec_adr->bottom_type()->is_ptr());
-      MergeMemNode* local_mem = MergeMemNode::make(mem);
-      gvn.record_for_igvn(local_mem);
-      BarrierSetC2* bs = BarrierSet::barrier_set()->barrier_set_c2();
-      C2OptAccess access(gvn, ctrl, local_mem, decorators, T_OBJECT, obj, addr);
-      const Type* type = TypeOopPtr::make_from_klass(field->type()->as_klass());
-      vec_field_ld = bs->load_at(access, type);
-    }
+    Node* vec_field_ld = kit.access_load_at(obj,
+                                            vec_adr,
+                                            vec_adr->bottom_type()->is_ptr(),
+                                            TypeOopPtr::make_from_klass(field->type()->as_klass()),
+                                            T_OBJECT,
+                                            C2_READ_ACCESS | C2_CONTROL_DEPENDENT_LOAD | IN_HEAP);
 
     // For proper aliasing, attach concrete payload type.
     ciKlass* payload_klass = ciTypeArrayKlass::make(bt);

@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Jan 29, 2021

ArrayCopyNode::load performs the same work as it does here in PhaseVector::optimize_vector_boxes .
Is there a need to provide a similar function in PhaseVector or GraphKit?

My point is since PhaseVector effectively enters the parsing phase (by signaling about the possibility of post-parse inlining), technically I don't see why GraphKit::access_load_at won't work. But I need to spend more time looking into the details.

So far, I took a look at the review thread of 8212243 (which introduced ArrayCopyNode::load) and found the following discussion between Roland and Erik:
https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2018-October/030971.html

> ... Also it beats  me that this is strictly speaking a load barrier for loads performed in 
> arraycopy. Would it be possible to use something like access_load_at  instead? ...
...
GraphKit is a parse time only thing. So the existing gc interface
doesn't offer any way to add barriers once parsing is over. This code
runs after parsing in optimization phases.
...

Considering PhaseVector::optimize_vector_boxes() already has access to a usable GraphKit instance, it is possible that GraphKit::access_load_at will "just work".

As far as I can see, during the parse phase, GraphKit contains the jvm state info which can be used to get the control and memory for creating new nodes. But during optimization, the jvm state info may be missing like the situation in PhaseVector::optimize_vector_boxes or Macro Expansion. So it should use C2OptAccess to create the Load Node directly by providing control and memory nodes.

I think a similar api like GraphKit::access_load_at should be provided for usage during optimization stages, but where should the API be placed? GraphKit or PhaseIterGVN or somewhere else?

@fisk
Copy link
Contributor

@fisk fisk commented Jan 29, 2021

Considering PhaseVector::optimize_vector_boxes() already has access to a usable GraphKit instance, it is possible that GraphKit::access_load_at will "just work".

The main thing to make sure you get right, is the aliasing. I'm not sure that will work right after parsing, the way it works now.

@neliasso
Copy link
Contributor

@neliasso neliasso commented Jan 29, 2021

I suggest you keep this CR as it is since 16 is in rampdown and we need to get approval and push it before Feb 4th (and we do want some margin).

Open an enhancement on 17 to fix the api.

vec_field_ld = gvn.transform(vec_field_ld);
Node* vec_field_ld;
{
DecoratorSet decorators = C2_READ_ACCESS | C2_CONTROL_DEPENDENT_LOAD | IN_HEAP;
Copy link
Contributor

@neliasso neliasso Jan 29, 2021

C2_READ_ACCESS will be set by "bs->load_at" so you can skip that.
MO_UNORDERED is missing. That corresponds to "MemNode::unordered" in the original code.

C2_CONTROL_DEPENDENT_LOAD is also redundant (though original code does that): it's just a plain load from a final instance field).

Copy link
Contributor Author

@casparcwang casparcwang Jan 30, 2021

Fixed in the new pr: openjdk/jdk16#139

@iwanowww
Copy link

@iwanowww iwanowww commented Jan 29, 2021

I suggest you keep this CR as it is since 16 is in rampdown and we need to get approval and push it before Feb 4th (and we do want some margin).

I agree. @casparcwang, please, file an RFE.

@iwanowww
Copy link

@iwanowww iwanowww commented Jan 29, 2021

As far as I can see, during the parse phase, GraphKit contains the jvm state info which can be used to get the control and memory for creating new nodes. But during optimization, the jvm state info may be missing like the situation in PhaseVector::optimize_vector_boxes or Macro Expansion.

JVM state is irrelevant here (otherwise, VectorUnbox node would have captured relevant info during construction). What is actually missing is GraphKit instance lacks info about control and memory. You need to explicitly set it using GraphKit::set_control() and GraphKit::set_all_memory().

@neliasso
Copy link
Contributor

@neliasso neliasso commented Jan 29, 2021

We need this patch to be based on the JDK 16 repository.

I will help out with the fix-request and sponsor-ship.

@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Jan 30, 2021

As far as I can see, during the parse phase, GraphKit contains the jvm state info which can be used to get the control and memory for creating new nodes. But during optimization, the jvm state info may be missing like the situation in PhaseVector::optimize_vector_boxes or Macro Expansion.

JVM state is irrelevant here (otherwise, VectorUnbox node would have captured relevant info during construction). What is actually missing is GraphKit instance lacks info about control and memory. You need to explicitly set it using GraphKit::set_control() and GraphKit::set_all_memory().

Thank you for the explanation. @iwanowww

We need this patch to be based on the JDK 16 repository.

I will help out with the fix-request and sponsor-ship.

Thank you very much. @neliasso
I have create a new patch based on JDK16 repo: openjdk/jdk16#139

@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Jan 30, 2021

I suggest you keep this CR as it is since 16 is in rampdown and we need to get approval and push it before Feb 4th (and we do want some margin).

I agree. @casparcwang, please, file an RFE.

Jie Fu @DamonFool has helped to create an RFE. https://bugs.openjdk.java.net/browse/JDK-8260682

@casparcwang
Copy link
Contributor Author

@casparcwang casparcwang commented Feb 1, 2021

Thanks @iwanowww @neliasso @pliden @stooart-mo @XiaohongGong @fisk @DamonFool for the reviews and helping.
The patch has integrated in jdk16 (openjdk/jdk16#139), and this pr should be closed.

@casparcwang casparcwang closed this Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment