JIT common and x86 support for constant dynamic #2754

yanluo7 · 2018-08-30T19:19:15Z

Signed-off-by: Yan Luo Yan_Luo@ca.ibm.com

yanluo7 · 2018-08-30T19:20:18Z

OMR dependency: eclipse-omr/omr#2912
Design documentation is detailed in #2307

yanluo7 · 2018-08-30T19:36:30Z

@cathyzhyi Could you do a review?

yanluo7 · 2018-08-31T17:17:26Z

Travis CI build failure is expected due to OMR dependency change.

cathyzhyi · 2018-09-10T18:30:17Z

runtime/compiler/env/j9method.cpp

+// Else if second slot is Void.class, the CP entry is resolved to null (0) value.
+// Other casese, the CP entry is considered unresolved.
+bool
+TR_ResolvedJ9Method::isUnresolvedConstantDynamic(I_32 cpIndex)


Does the first slot represent the value of the condy and the second slot represent the resolved class of the condy? Maybe it's better to say this explicitly in the comment to avoid confusion especially when the name of the second slot is called exception.

Both value and exception slots are of object pointer type. If first slot is non null, the CP entry is resolved to a non-null value. Else if second slot is the class object of j/l/Void, the CP entry is resolved to null (0) value. We retrieve the Void class object via javaVM->voidReflectClass->classObject, which is protected by VMAccessCrtiticalSection to ensure vm access. Other cases, the CP entry is considered unresolved. Added VMAccessCrtiticalSection code and more comments.

irinarada · 2018-09-11T12:31:06Z

@yanluo7 , @cathyzhyi any ETA for the latest changes? This blocks the z delivery part. @fjeremic , @gita-omr @andrewcraik

andrewcraik · 2018-09-11T17:48:54Z

@yanluo7 asked me to review this yesterday - I have just arrived back from vacation so I will try and do a review today. If the review goes well the merge should be very soon, but I would target end of the week.

andrewcraik · 2018-09-11T20:05:02Z

runtime/compiler/env/j9method.cpp

+bool
+TR_ResolvedJ9Method::isConstantDynamic(I_32 cpIndex)
+   {
+   TR_ASSERT(cpIndex != -1, "cpIndex shouldn't be -1");


This should probably be an TR_ASSERT_FATAL - it will never work with cpIndex -1, it doesn't handle the case, and it is just plain wrong to call it with -1. The check is cheap so keeping it in at runtime will make sure we don't miss a bad use of this API.

Yes, this method should have fatal assertion as this query is the entry point of JIT condy processing logic.

andrewcraik · 2018-09-11T20:05:44Z

runtime/compiler/env/j9method.cpp

+void *
+TR_ResolvedJ9Method::dynamicConstant(I_32 cpIndex)
+   {
+   TR_ASSERT(cpIndex != -1, "cpIndex shouldn't be -1");


same thing for this one

Added fatal assertion here.

andrewcraik · 2018-09-11T20:17:37Z

runtime/compiler/ilgen/Walker.cpp

+               {
+               char *autoboxClassSig = NULL;
+               int32_t autoboxClassSigLength = 0;
+               switch (returnTypeUtf8Data[0])


There should be an assert above this to make sure the signature is valid - ie 1 char.

Here we are inside the if (isCondyPrimitive) condition so it's already implicitly ensured that turnTypeUtf8Length is 1 for the type signature UTF8 : bool isCondyPrimitive = (1 == returnTypeUtf8Length);

andrewcraik · 2018-09-11T20:17:51Z

runtime/compiler/ilgen/Walker.cpp

+                     valueOffset = comp()->fej9()->getObjectHeaderSizeInBytes()
+                                          + comp()->fej9()->getInstanceFieldOffset(typeClassBlock, "value", 5, "Z", 1);
+                     break;
+                  default:


shouldn't this assert?

Yes, added a fatal assertion for unrecognized type signature.

andrewcraik · 2018-09-11T20:19:24Z

@yanluo7 the new methods need doxygen comments to describe their operation, their inputs and their return values as well as anything else people using the API need to know / be aware of

andrewcraik · 2018-09-12T20:16:30Z

runtime/compiler/ilgen/Walker.cpp

+                           loadConstant(TR::dconst, *(double*)(obj+valueOffset));
+                           break;
+                        default:
+                           break;


need an assert here too to match the case above.

Added assertion.

andrewcraik · 2018-09-12T20:16:45Z

runtime/compiler/ilgen/Walker.cpp

+                     dt = TR::Int32;
+                     break;
+                  default:
+                     break;


Also needs an assert

Added assertion.

andrewcraik · 2018-09-12T20:31:56Z

Jenkins test sanity all jdk11

andrewcraik · 2018-09-12T20:32:23Z

Ok I think that looks good - will let sanity run and then I think this should be good to go.

fjeremic · 2018-09-12T20:59:44Z

We're waiting for an OMR-Acceptance promotion due to a bad OMR merge causing mutual dependencies for Z codny support. Sorry about that. You can launch a Jenkinds depends build against eclipse/omr#master though. That should work fine.

andrewcraik · 2018-09-12T21:03:18Z

Jenkins test sanity all jdk11 depends eclipse/omr#master

andrewcraik · 2018-09-12T21:03:45Z

@fjeremic thanks we'll do that - my bad for not noticing since I've been busy with non-openj9 things

Signed-off-by: Yan Luo <Yan_Luo@ca.ibm.com>

yanluo7 · 2018-09-13T00:42:54Z

Added missing ExternHelper declaration that should address windows compile.

andrewcraik · 2018-09-13T01:16:06Z

Jenkins test sanity all jdk11 depends eclipse/omr#master

andrewcraik · 2018-09-13T07:15:58Z

@yanluo7 can you take a look at sanity and see if the failures are of concern in your opinion - I will look at them separately and see if we agree

yanluo7 · 2018-09-13T15:20:15Z

@andrewcraik
The test failures are summarized here:
win_x86-64_cmprssptrs & linux_x86-64_cmprssptrs : Nestmate_virtual_private_0
linux_x86-64: jsr335_interfacePrivateMethod and Nestmate_virtual_private
linux_390-64_cmprssptrs : org.openj9.test.java.lang.Test_String.test_toLowerCase_lithuanian and
java.lang.ClassFormatError: JVMCFRE145 Nest host must be a constant class; There seems to be a few NEST-related failures here.

The entry point to all the constant dynamic logic gated by TR_ResolvedJ9Method::isConstantDynamic(cpIndex), which checks whether the given CP type is J9CPTYPE_CONSTANT_DYNAMIC , it should not kick in for all of these tests as they don't have constant dynamics in them.

andrewcraik · 2018-09-14T17:45:57Z

@yanluo7 I agree - there don't seem to be constant dynamic failures in the above so I will merge this.

yanluo7 mentioned this pull request Aug 30, 2018

ConstantDynamic: JIT support for load constant dynamic #2307

Closed

yanluo7 force-pushed the condy branch from 690e026 to 7f74e68 Compare August 30, 2018 19:32

yanluo7 mentioned this pull request Aug 30, 2018

JIT common and x86 support for constant dynamic eclipse-omr/omr#2912

Merged

DanHeidinga added the depends:omr Pull request is dependent on a corresponding change in OMR label Sep 5, 2018

yanluo7 force-pushed the condy branch 6 times, most recently from 10fe200 to e3ae231 Compare September 9, 2018 15:45

cathyzhyi reviewed Sep 10, 2018

View reviewed changes

r30shah mentioned this pull request Sep 10, 2018

IBM Z Codegen changes to support Condy in jdk11 #2806

Merged

yanluo7 force-pushed the condy branch 3 times, most recently from 70f9541 to 140cf1b Compare September 11, 2018 16:05

andrewcraik reviewed Sep 11, 2018

View reviewed changes

yanluo7 force-pushed the condy branch 2 times, most recently from 37c2667 to 1a803cb Compare September 12, 2018 15:56

andrewcraik reviewed Sep 12, 2018

View reviewed changes

yanluo7 force-pushed the condy branch from 1a803cb to 5562865 Compare September 12, 2018 20:26

JIT support for constant dynamic

d574214

Signed-off-by: Yan Luo <Yan_Luo@ca.ibm.com>

yanluo7 force-pushed the condy branch from 5562865 to d574214 Compare September 13, 2018 00:39

andrewcraik merged commit 63bd3cc into eclipse-openj9:master Sep 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT common and x86 support for constant dynamic #2754

JIT common and x86 support for constant dynamic #2754

yanluo7 commented Aug 30, 2018

yanluo7 commented Aug 30, 2018 •

edited

Loading

yanluo7 commented Aug 30, 2018

yanluo7 commented Aug 31, 2018

cathyzhyi Sep 10, 2018

yanluo7 Sep 11, 2018

irinarada commented Sep 11, 2018

andrewcraik commented Sep 11, 2018

andrewcraik Sep 11, 2018

yanluo7 Sep 12, 2018

andrewcraik Sep 11, 2018

yanluo7 Sep 12, 2018

andrewcraik Sep 11, 2018

yanluo7 Sep 12, 2018

andrewcraik Sep 11, 2018

yanluo7 Sep 12, 2018

andrewcraik commented Sep 11, 2018

andrewcraik Sep 12, 2018

yanluo7 Sep 12, 2018

andrewcraik Sep 12, 2018

yanluo7 Sep 12, 2018

andrewcraik commented Sep 12, 2018

andrewcraik commented Sep 12, 2018

fjeremic commented Sep 12, 2018

andrewcraik commented Sep 12, 2018

andrewcraik commented Sep 12, 2018

yanluo7 commented Sep 13, 2018

andrewcraik commented Sep 13, 2018

andrewcraik commented Sep 13, 2018

yanluo7 commented Sep 13, 2018 •

edited

Loading

andrewcraik commented Sep 14, 2018

JIT common and x86 support for constant dynamic #2754

JIT common and x86 support for constant dynamic #2754

Conversation

yanluo7 commented Aug 30, 2018

yanluo7 commented Aug 30, 2018 • edited Loading

yanluo7 commented Aug 30, 2018

yanluo7 commented Aug 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irinarada commented Sep 11, 2018

andrewcraik commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewcraik commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewcraik commented Sep 12, 2018

andrewcraik commented Sep 12, 2018

fjeremic commented Sep 12, 2018

andrewcraik commented Sep 12, 2018

andrewcraik commented Sep 12, 2018

yanluo7 commented Sep 13, 2018

andrewcraik commented Sep 13, 2018

andrewcraik commented Sep 13, 2018

yanluo7 commented Sep 13, 2018 • edited Loading

andrewcraik commented Sep 14, 2018

yanluo7 commented Aug 30, 2018 •

edited

Loading

yanluo7 commented Sep 13, 2018 •

edited

Loading