JIT common and x86 support for constant dynamic

[yanluo7]

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

[yanluo7]

openj9 dependency: eclipse-openj9/openj9#2754
Design documentation is detailed in eclipse-openj9/openj9#2307

[yanluo7]

@cathyzhyi Could you do a review?

[cathyzhyi]

LGTM

[vijaysun-omr]

Could you please elaborate on this condition ?

"is constant dynamic OR !is constant object ref" seems like an odd combination...

[yanluo7]

Previously, if the symbol is true for isConstObjectRef, we would not return the conservative aliasing BV here, which in this place, is opposite to what we want for ConstantDynamic. On the other hand, the query isConstObjectRef is used in other places in the JIT and upon examining each of these uses, we believe ConstantDynamic should also be included as part of the query instead of explicit ORing them isConstantDynamic() || isConstObjectRef(). So in the few exception cases here in Aliases.cpp, we made the logic that says in case of isConstObjectRef. don't return the conservative aliasing unless it's condy.

[fjeremic]

That seems... confusing. I would think it would warrant a more elaborated explanation within a comment in the code.

[vijaysun-omr]

I'm still a bit unclear after the explanation. Two questions that come to mind:

1. You said "if the symbol is true for isConstObjectRef, we would not return the conservative aliasing BV here, which in this place, is opposite to what we want for ConstantDynamic." Why do we want the opposite behaviour for constant dynamic here ?

2. You said "isConstObjectRef is used in other places in the JIT and upon examining each of these uses, we believe ConstantDynamic should also be included as part of the query instead of explicit ORing them". To be completely clear does this mean that we return true for a constant dynamic symbol reference when we call isConstObjectRef ? If the answer to that question is "yes" then I don't understand how the logical OR can achieve the desired result you mention in your comment "So in the few exception cases here in Aliases.cpp, we made the logic that says in case of isConstObjectRef. don't return the conservative aliasing unless it's condy." ... I'd have thought you would have needed a logical AND instead, i.e. if isConstObjectRef && !isConstantDynamic then do the original behaviour for const object ref at this spot in the code. I'm probably missing something...

[yanluo7]

1. For unresolved condy, we need to invoke a Java bootstrap method, which can have arbitrary side effects, so the aliasing should be conservative here.

2. Yes to returning true for a constant dynamic symbol reference when we call isConstObjectRef.
   We added |ConstantDynamic in the _flags2.testAny check:

bool
OMR::Symbol::isConstObjectRef()
    {
    return self()->isStatic() && (_flags.testAny(ConstString)  || 
         _flags2.testAny(ConstMethodType|ConstMethodHandle|ConstantDynamic));
    }

In the JIT code, there are quite a few uses of isConstObjectRef where ConstantDynamic should also apply, e.g.:

   if (isConstObjectRef())
       // certain inliner heuristic

So by incorporating ConstantDynamic into the isConstObjectRef query, we don't need to change all of those places.

The exception cases are in Alaises.cpp. In this place previously the logic was:

 if (self()->isUnresolved() && !_symbol->isConstObjectRef() 
        || ...)
            {
            return &comp->getSymRefTab()->aliasBuilder.defaultMethodDefAliases();
            }

If we are unresolved and not isConstObjectRef, we return the conservative defaultMethodDefAliases, and defaultMethodDefAliases is the desired for condy.
But given that isConstObjectRef now returns true for condy, this logic as-is would not work for condy. So we add an explicit condition, more like a short-circuit, to say if we are unresolved and not isConstObjectRef (this is the same as before), or if we are unresolved and condy (this is the extra condition added), we would return conservative aliases.

[vijaysun-omr]

Okay thanks for the explanation. Please add some version of this explanation to the code comments as Filip requested.

[mstoodle]

genie-omr build all