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

TransformIndirectLoadChain at JITServer #20767

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

luke-li-2003
Copy link
Contributor

Implement TransformIndirectLoadChain partially for the JITServer so it can employ the Vector API during optimization.

@luke-li-2003 luke-li-2003 force-pushed the TransformIndirectLoadChainAtServer branch 2 times, most recently from ed4300a to 8eef209 Compare December 5, 2024 21:38
@luke-li-2003
Copy link
Contributor Author

@mpirvu

@mpirvu mpirvu self-assigned this Dec 5, 2024
@mpirvu mpirvu added comp:jit comp:jitserver Artifacts related to JIT-as-a-Service project labels Dec 5, 2024
Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

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

Is it possible to do some refactoring to avoid code duplication? The risk is that any fixes to dereferenceStructPointerChain() or verifyFieldAccess() are not going to be ported to transformIndirectLoadChainServerImpl

@luke-li-2003
Copy link
Contributor Author

Is it possible to do some refactoring to avoid code duplication?

It is possible, the main challenge is that the non-server implementation assumes VM access throughout the code, so we need to do some major re-writing.

@mpirvu
Copy link
Contributor

mpirvu commented Dec 10, 2024

How about moving all the JITServer changes down into dereferenceStructPointerChain.
So create a new, much simpler variant, that only handles some case for remote compilations. Something like this:

static void *dereferenceStructPointer(TR::KnownObjectTable::Index baseKnownObjectIndex, TR::Node *baseNode, bool isBaseStableArray, TR::Node *curNode, TR::Compilation *comp, void *valuePtr)
   {
   if (baseNode == curNode)
      {
      TR_ASSERT(false, "dereferenceStructPointerChain has no idea what to dereference");
      traceMsg(comp, "Caller has already dereferenced node %p, returning NULL as dereferenceStructPointerChain has no idea what to dereference\n", curNode);
      return NULL;
      }
   else
      {
      TR_ASSERT(curNode != NULL, "Field node is NULL");
      TR_ASSERT(curNode->getOpCode().hasSymbolReference(), "Node must have a symref");

      TR::SymbolReference *symRef = curNode->getSymbolReference();
      TR::Symbol *symbol = symRef->getSymbol();
      TR::Node *addressChildNode = symbol->isArrayShadowSymbol() ? curNode->getFirstChild()->getFirstChild() : curNode->getFirstChild();

      // The addressChildNode must has a symRef so that we can verify it
      if (!addressChildNode->getOpCode().hasSymbolReference())
         return NULL;

      if (isBaseStableArray)
         TR_ASSERT_FATAL(addressChildNode == baseNode, "We should have only one level of indirection for stable arrays\n");

      if (addressChildNode == baseNode)
         {
        // If baseStruct/baseNode and deemed verifiable by the caller do we still need to verify them?
        // Part of the verification. Only Java fields are considered
        if (isJavaField(symRef, comp)) // symbol->isShadow() && (cpIndex >= 0 || symbol->getRecognizedField() != TR::Symbol::UnknownField)
            {
            TR_OpaqueClassBlock *fieldClass = NULL;
            // Fabricated fields don't have valid cp index
            if (field->getCPIndex() < 0 &&
                field->getSymbol()->getRecognizedField() != TR::Symbol::UnknownField)
                {
                const char* className;
                int32_t length;
                className = field->getSymbol()->owningClassNameCharsForRecognizedField(length);
                fieldClass = fej9->getClassFromSignature(className, length, field->getOwningMethod(comp));
                }
            else
                fieldClass = field->getOwningMethod(comp)->getDeclaringClassFromFieldOrStatic(comp, field->getCPIndex());

            if (fieldClass == NULL)
                return NULL;
            TR_OpaqueClassBlock *objectClass =fej9->getObjectClassFromKnownObjectIndex(comp, baseKnownObjectIndex)
            TR_YesNoMaybe objectContainsField = fej9->isInstanceOf(objectClass, fieldClass, true);
            if (objectContainsField != TR_yes)
                return NULL;
            if (TR::TransformUtil::avoidFoldingInstanceField(baseStruct, symRef, comp)) // This needs some work
                {
                if (comp->getOption(TR_TraceOptDetails))
                    {
                    traceMsg(comp, "avoid folding load of field #%d from object at index %d\n", symRef->getReferenceNumber(), baseKnownObjectIndex);
                    }
                return NULL;
                }
            
            // (void*)comp->getKnownObjectTable()->getPointer(baseKnownObjectIndex)
            // value stored at fieldAddress from object and offset  fieldAddress = curStruct + symRef->getOffset(); if (!fieldAddress) return NULL
            // if (isCollectedReference())
            // value = fej9->getReferenceFieldAtAddress((uintptr_t)fieldAddress);  //==> Needs VM acess
            uint64_t data = sendMessageToServer(baseKnownObjectIndex, symRef->getOffset()); // Should be big enough to hold a double, a 64-bit thing and a pointer; also should have a representation for noData
            // pass value back to the caller
            TR::DataType loadType = node->getDataType();
            switch (loadType)
               {
               case TR::Int32:
                 *(int32_t*)valuePtr = (int32_t)data;
                 break;
             .....
               case TR::Address:
                  *(uintptr_t*)valuePtr = (uintptr_t)data;
                  break;
               }
            return valuePtr;
            }
         }
      }
   return NULL;
   }

This one, on failure it returns NULL, much like the original dereferenceStructPointerChain. On success it returns a pointer to where the data is going to be found.
So the caller uses a union for all data types possible:

union {
    int32_t i;
    int64_t l;
    float f;
    double d;
    void *p;
} value;

and it calls void *fieldAddress = dereferenceStructPointerChain(baseAddress, baseExpression, isBaseStableArray, node, comp, &value);
So, fieldAddress and &value are going to be the same.
Then the caller continues with the original code, but isCollectedReference case it should avoid doing uintptr_t value = fej9->getReferenceFieldAtAddress((uintptr_t)fieldAddress); because the client has already obtained that value.

@luke-li-2003
Copy link
Contributor Author

I have yet to implement the union, but I just want to make sure the overall structure of the code is correct.

@luke-li-2003 luke-li-2003 force-pushed the TransformIndirectLoadChainAtServer branch from e4c608a to eeebb5d Compare December 10, 2024 23:36
@luke-li-2003
Copy link
Contributor Author

I have yet to organise the commits. Vector API now works for both server and non-server mode. I also had to bypass transformIndirectChainAt because I am not sure how to implement it yet.

@luke-li-2003 luke-li-2003 force-pushed the TransformIndirectLoadChainAtServer branch from eeebb5d to c7311bc Compare December 10, 2024 23:38
runtime/compiler/optimizer/J9TransformUtil.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/J9TransformUtil.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/J9TransformUtil.cpp Outdated Show resolved Hide resolved
@luke-li-2003 luke-li-2003 force-pushed the TransformIndirectLoadChainAtServer branch 2 times, most recently from ea629a8 to 8da6728 Compare December 11, 2024 22:28
runtime/compiler/optimizer/J9TransformUtil.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/J9TransformUtil.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/J9TransformUtil.cpp Outdated Show resolved Hide resolved
runtime/compiler/optimizer/J9TransformUtil.cpp Outdated Show resolved Hide resolved
runtime/compiler/control/JITClientCompilationThread.cpp Outdated Show resolved Hide resolved
@luke-li-2003 luke-li-2003 force-pushed the TransformIndirectLoadChainAtServer branch 9 times, most recently from c39d9a2 to 62b720a Compare December 13, 2024 18:11
@luke-li-2003 luke-li-2003 force-pushed the TransformIndirectLoadChainAtServer branch from 62b720a to 176c759 Compare December 16, 2024 19:03
isArrayWithConstantElements(symRef, comp));
}

if (knotIndex != TR::KnownObjectTable::UNKNOWN)
Copy link
Contributor

Choose a reason for hiding this comment

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

The original code has a test on value being non-null. Is this test equivalent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should be

@luke-li-2003
Copy link
Contributor Author

With addArrayWithConstantElements being protected, I have to involve a change in omr as well

eclipse-omr/omr#7594

@mpirvu
Copy link
Contributor

mpirvu commented Dec 17, 2024

I am thinking that we can delete OMR::KnownObjectTable::updateKnownObjectTableAtServer(Index index, uintptr_t *objectReferenceLocation) from OMR since it's only used in OpenJ9

@mpirvu
Copy link
Contributor

mpirvu commented Dec 17, 2024

jenkins compile xlinux jdk21

@mpirvu
Copy link
Contributor

mpirvu commented Dec 17, 2024

jenkins compile all jdk8,jdk21

Implement TransformIndirectLoadChain partially for the JITServer
so it can employ the Vector API during optimization.

Signed-off-by: Luke Li <[email protected]>
@luke-li-2003 luke-li-2003 force-pushed the TransformIndirectLoadChainAtServer branch from 150fa69 to ee700f1 Compare December 17, 2024 18:23
@luke-li-2003 luke-li-2003 marked this pull request as ready for review December 17, 2024 18:52
@mpirvu
Copy link
Contributor

mpirvu commented Dec 17, 2024

jenkins test sanity all jdk23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:jit comp:jitserver Artifacts related to JIT-as-a-Service project
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

2 participants