Implement X86 codegen support for dynamic array clone #4179

yanluo7 · 2019-01-04T20:56:13Z

Dynamic array cloning transforms refArrayObject.clone()
into anewarray followed by arraycopy, where refArrayObject
is a resolved, not fixed class. For code gen, It requires the
class (2nd) child of anewarray to be runtime evaluated instead
of compile time constant, as actual refArrayObject type is not
known at compiled time (other than it's an ref array). This
change implements support in X86 codegen to handle a runtime
evaluated class child of anewarray.

This is prerequisite for the optimizer change that performs the
object.clone() call transformation.

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

yanluo7 · 2019-01-04T21:38:39Z

The desired tree after transformation looks like following:
Note the aloadi <componentClass> child of anewarray node where we obtain the component class from the class of the receiver array object of object.clone() :

treetop ()
  anewarray  jitANewArray
    iloadi  <contiguous-array-size>
      ==>l2a (array object to be cloned)
    aloadi  <componentClass>
      ==>aloadi (vftload of array object)
treetop ()
  arraycopy  <arraycopy>
    ==>l2a
    ==>anewarray
    aladd
      ==>l2a
      lconst 8
    aladd 
      ==>anewarray
      ==>lconst 8
    lshl 
       i2l 
         ==>iloadi
       iconst 2

andrewcraik · 2019-01-04T21:48:30Z

runtime/compiler/x/codegen/J9TreeEvaluator.cpp

+   // the original array object (array object to be copied). i.e. we are loading the component class from the original array class
+   // and pass the component class as 2nd child of anewarray. So the 2nd child must be evaluated into a reg.
+   bool dynamicArrayClone = (node->getOpCodeValue() == TR::anewarray) && (node->getSecondChild()->getSymbolReference()) &&
+         (node->getSecondChild()->getSymbolReference() == comp->getSymRefTab()->findArrayComponentTypeSymbolRef());


this seems a little optimistic - shouldn't we say that if it isn't a loadaddr (or possibly aconst) then it is dynamic and assert that we have clazz if the array is not dynamic?

Yes, added extra condition that checks for the class node op code for dynamic array, also assertion that rensure clazz cannot be null when not dynamic array.

This still seems pretty special-purpose. Do we need to require that the component type child be either constant or in this restricted form? Or could we use something like the following?

node->getOpCodeValue() == TR::anewarray && clazz == NULL

I think canAllocateInline() could set clazz = NULL whenever the child isn't loadaddr.

I revised the logic for evaluating the isDynamicArray boolean. Now we only initialize the boolean once in the top level NewEvaluator using the return values from canAllocateInline(). This way checking the ArrayComponentType symbol occurs only once in canAllocateInline(). And the NewEvaluator passes this flag down to helper routines which only need to check the flag to do the right thing. Currently the boolean is named as array-specific since only dynamic reference array allocations are supported for now. When we implement dynamic allocation for other object we can rename it properly -- just isDynamicAllocation.

andrewcraik · 2019-01-04T21:49:05Z

runtime/compiler/x/codegen/J9TreeEvaluator.cpp


-         TR_X86OpCodes op = (TR::Compiler->target.is64Bit() && fej9->generateCompressedLockWord()) ? S4MemImm4 : SMemImm4();
-         generateMemImmInstruction(op, node, generateX86MemoryReference(objectReg, lwOffset, cg), initialLwValue, cg);
+         if (initLw)


I think some comments about lock word handling are warranted along with a description of the interaction with lock reservation

Added more comments to address lock reservation case.

andrewcraik · 2019-01-07T17:58:59Z

@0dvictor could I get your thoughts on this please?

0dvictor · 2019-01-07T19:47:26Z

runtime/compiler/x/codegen/J9TreeEvaluator.cpp

+#if defined (J9VM_THR_LOCK_NURSERY)
+      generateRegMemInstruction(LRegMem(), node, tempReg, generateX86MemoryReference(clzReg, offsetof(J9ArrayClass, lockOffset), cg), cg);
+#else
+      generateRegImmInstruction(MOVRegImm4(), node, tempReg, TMP_OFFSETOF_J9OBJECT_MONITOR, cg);


In this case, tempReg holds a compile-time constant, so that the branching logic below can be fold away.

Removed the !J9VM_THR_LOCK_NURSERY case altogether. The flag is set on all platforms. Also modified VP to not generate dynamic objects if flag is not set.

0dvictor · 2019-01-07T19:51:29Z

runtime/compiler/x/codegen/J9TreeEvaluator.cpp

+      generateRegImmInstruction(MOVRegImm4(), node, tempReg, TMP_OFFSETOF_J9OBJECT_MONITOR, cg);
+#endif
+      generateRegImmInstruction(CMPRegImm4(), node, tempReg, (int32_t)-1, cg);
+      generateLabelInstruction (JE4, node, doneLabel, true, cg);


generateLabelInstruction with needsVMThreadRegister parameter has been deprecated; hence this line should be:
generateLabelInstruction (JE4, node, doneLabel, cg);

0dvictor · 2019-01-07T19:53:09Z

runtime/compiler/x/codegen/J9TreeEvaluator.cpp

+      generateRegImmInstruction(CMPRegImm4(), node, tempReg, (int32_t)-1, cg);
+      generateLabelInstruction (JE4, node, doneLabel, true, cg);
+      generateRegMemInstruction(LEARegMem(), node, tempReg, generateX86MemoryReference(objectReg, tempReg, 0, cg) ,cg);
+      TR_X86OpCodes op = (TR::Compiler->target.is64Bit() && fej9->generateCompressedLockWord()) ? S4MemImm4 : SMemImm4();


This logic may be simplified as:
SMemImm4(TR::Compiler->target.is64Bit() && !fej9->generateCompressedLockWord())

0dvictor · 2019-01-07T19:54:22Z

runtime/compiler/x/codegen/J9TreeEvaluator.cpp

+            if (initReservable)
+               initialLwValue = OBJECT_HEADER_LOCK_RESERVED;
+
+            TR_X86OpCodes op = (TR::Compiler->target.is64Bit() && fej9->generateCompressedLockWord()) ? S4MemImm4 : SMemImm4();


This logic may be simplified as:
SMemImm4(TR::Compiler->target.is64Bit() && !fej9->generateCompressedLockWord())

jdmpapin · 2019-01-07T20:04:14Z

runtime/compiler/x/codegen/J9TreeEvaluator.cpp

+         {
+         clzReg = tempReg;
+         generateRegMemInstruction(LRegMem(), node, clzReg, generateX86MemoryReference(classReg, offsetof(J9Class, arrayClass), cg), cg);
+         }	      


As it stands, this will generate the load twice if dynamic array clone occurs in "old" AOT. Could it be combined with the path that generates the corresponding load above? e.g.

if (classReg != NULL && node->getOpCodeValue() == TR::anewarray) { // Variable component type, due either to dynamicArrayClone or (non-SVM) AOT. // Load the array type from it at runtime. clzReg = tempReg; generateRegMemInstruction(LRegMem(), node, clzReg, generateX86MemoryReference(classReg, offsetof(J9Class, arrayClass), cg), cg); } else if (cg->needClassAndMethodPointerRelocations() && !comp->getOption(TR_UseSymbolValidationManager)) { // ... [check for TR::newarray and TR::New, since TR::anewarray has been ruled out] ... }

Revised the logic as suggested.

jdmpapin · 2019-01-07T20:28:25Z

runtime/compiler/x/codegen/J9TreeEvaluator.cpp

+   // the original array object (array object to be copied). i.e. we are loading the component class from the original array class
+   // and pass the component class as 2nd child of anewarray. So the 2nd child must be evaluated into a reg.
+   bool dynamicArrayClone = (node->getOpCodeValue() == TR::anewarray) && (node->getSecondChild()->getSymbolReference()) &&
+         (node->getSecondChild()->getSymbolReference() == comp->getSymRefTab()->findArrayComponentTypeSymbolRef());


This still seems pretty special-purpose. Do we need to require that the component type child be either constant or in this restricted form? Or could we use something like the following?

node->getOpCodeValue() == TR::anewarray && clazz == NULL

I think canAllocateInline() could set clazz = NULL whenever the child isn't loadaddr.

jdmpapin · 2019-01-07T20:37:35Z

runtime/compiler/x/codegen/J9TreeEvaluator.cpp

+      generateLabelInstruction (JE4, node, doneLabel, true, cg);
+      generateRegMemInstruction(LEARegMem(), node, tempReg, generateX86MemoryReference(objectReg, tempReg, 0, cg) ,cg);
+      TR_X86OpCodes op = (TR::Compiler->target.is64Bit() && fej9->generateCompressedLockWord()) ? S4MemImm4 : SMemImm4();
+      generateMemImmInstruction(op, node, generateX86MemoryReference(tempReg, 0, cg), 0, cg);


Can we get away without the lea just above, and generate

mov ... ptr [rObject + rTemp], 0

instead of

lea rTemp, [rObject + rTemp] mov ... ptr [rTemp], 0

?

andrewcraik · 2019-01-10T13:55:22Z

@yanluo7 could you comment on what has changed and respond to the review questions above so we can continue reviewing this change?

yanluo7 · 2019-01-10T14:56:45Z

@andrewcraik yes, forgot to hit submit

andrewcraik · 2019-01-10T15:34:50Z

@0dvictor and @jdmpapin could you review the latest changes to see if they address your comments on this item previously? thanks!

jdmpapin · 2019-01-10T15:47:31Z

runtime/compiler/compile/J9Compilation.cpp

@@ -650,6 +650,14 @@ J9::Compilation::canAllocateInline(TR::Node* node, TR_OpaqueClassBlock* &classIn
      classRef      = node->getSecondChild();
      classSymRef   = classRef->getSymbolReference();

+      // In the case of dynamic array cloning, second child is the array component type load,
+      // return 0 indicating variable dynamic array allocation
+      if (classSymRef && (classRef->getOpCodeValue() == TR::aloadi) && (classSymRef == self()->getSymRefTab()->findArrayComponentTypeSymbolRef()))


This condition still requires aloadi <arrayComponentType>, but the following code assumes that classRef is a loadaddr. So this will break if we ever get a variable component type child with a different form. AFAICT the fix for such a situation would be to weaken this condition to classRef->getOpCodeValue() != TR::loadaddr (and probably also to move the getSymbolReference() call after the conditional, to a point where we know that classRef is in fact a loadaddr, and therefore that it has a symref). Is there any particular reason we shouldn't do that now?

More generic, done.

jdmpapin · 2019-01-10T15:54:08Z

runtime/compiler/x/codegen/J9TreeEvaluator.cpp

+   // For dynamic array allocation, load the array class from the component class and store into clzReg
+   if (isDynamicAllocation)
+      {
+      clzReg = tempReg;


Can we assert here that node->getOpCodeValue() == TR::anewarray? Just because if (isDynamicAllocation) sounds more generic, but only anewarray is handled.

jdmpapin · 2019-01-10T16:18:55Z

runtime/compiler/x/codegen/J9TreeEvaluator.cpp

@@ -7293,7 +7314,7 @@ J9::X86::TreeEvaluator::VMnewEvaluator(
   TR_OpaqueClassBlock *clazz      = NULL;
   TR::Register        *classReg   = NULL;
   bool                 isArrayNew = false;
-
+   bool     dynamicArrayAllocation = false;


Since this isn't used between here and the real initialization below, would you mind moving the declaration down? Right now it looks like something might read this false

jdmpapin · 2019-01-10T16:32:23Z

runtime/compiler/x/codegen/J9TreeEvaluator.cpp

@@ -7697,6 +7723,7 @@ J9::X86::TreeEvaluator::VMnewEvaluator(
            dataOffset,
            tempReg,
            monitorSlotIsInitialized,
+            dynamicArrayAllocation,


If you pass true here, then the path in genInitObjectHeader() that loads the array type for "old" AOT can be deleted, since we'll start using the path that does it for dynamicArrayAllocation instead. Personally I think that would be more straightforward because effectively what old AOT is doing here is forcing the allocation to be dynamic

Good point.

0dvictor

LGTM

yanluo7 · 2019-01-10T21:10:21Z

@andrewcraik Victor and Devin have approved the change. If you're OK with it, this can be merged.

andrewcraik · 2019-01-11T14:27:41Z

Jenkins test sanity xlinux,win jdk8,jdk11

andrewcraik · 2019-01-11T14:27:51Z

once sanity is passed I'm good for this to go in

Dynamic array cloning transforms refArrayObject.clone() into `anewarray` followed by `arraycopy`, where refArrayObject is a resolved, not fixed class. For code gen, It requires the class (2nd) child of anewarray to be runtime evaluated instead of compile time constant, as actual refArrayObject type is not known at compiled time (other than it's an ref array). This change implements support in X86 codegen to handle a runtime evaluated class child of anewarray. This is prerequisite for the optimizer change that performs the object.clone() call transformation. Signed-off-by: Yan Luo <Yan_Luo@ca.ibm.com>

andrewcraik · 2019-01-14T16:37:54Z

Jenkins test sanity xlinux,win jdk8,jdk11

yanluo7 force-pushed the dynamicarrayclone_openj9 branch 2 times, most recently from 1f7bd99 to 7d04ad5 Compare January 4, 2019 21:19

andrewcraik reviewed Jan 4, 2019

View reviewed changes

DanHeidinga added comp:jit perf labels Jan 5, 2019

yanluo7 force-pushed the dynamicarrayclone_openj9 branch from 7d04ad5 to 27ea288 Compare January 5, 2019 19:32

yanluo7 mentioned this pull request Jan 7, 2019

Implement dynamic array clone in value propagation eclipse-omr/omr#3421

Merged

0dvictor suggested changes Jan 7, 2019

View reviewed changes

jdmpapin reviewed Jan 7, 2019

View reviewed changes

yanluo7 force-pushed the dynamicarrayclone_openj9 branch from 27ea288 to 2660839 Compare January 10, 2019 01:08

yanluo7 force-pushed the dynamicarrayclone_openj9 branch from 2660839 to 8f44d5e Compare January 10, 2019 16:23

jdmpapin reviewed Jan 10, 2019

View reviewed changes

yanluo7 force-pushed the dynamicarrayclone_openj9 branch from 8f44d5e to 316217a Compare January 10, 2019 17:48

jdmpapin approved these changes Jan 10, 2019

View reviewed changes

0dvictor approved these changes Jan 10, 2019

View reviewed changes

yanluo7 force-pushed the dynamicarrayclone_openj9 branch from 316217a to 0709034 Compare January 14, 2019 15:39

andrewcraik merged commit cd026c7 into eclipse-openj9:master Jan 14, 2019

andrewcraik mentioned this pull request Jul 12, 2019

Complete architecture support for dynamicANewArray #6441

Open

Implement X86 codegen support for dynamic array clone #4179

Implement X86 codegen support for dynamic array clone #4179

Conversation

yanluo7 commented Jan 4, 2019 • edited Loading

yanluo7 commented Jan 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanluo7 Jan 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewcraik commented Jan 7, 2019

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

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 Jan 10, 2019

yanluo7 commented Jan 10, 2019

andrewcraik commented Jan 10, 2019

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

0dvictor left a comment

Choose a reason for hiding this comment

yanluo7 commented Jan 10, 2019

andrewcraik commented Jan 11, 2019

andrewcraik commented Jan 11, 2019

andrewcraik commented Jan 14, 2019

yanluo7 commented Jan 4, 2019 •

edited

Loading

yanluo7 Jan 10, 2019 •

edited

Loading