Clean up x/env/OMRCPU class and add missing supportsHLE()

[harryyu1994]

• Remove unused parameter (TR::Compilation *comp)
• Add missing supportsHLE() to TR_X86ProcessorInfo
• Remove J9_PROJECT_SPECIFIC macro in OMR::X86::CPU::queryX86TargetCPUID()

Signed-off-by: Harry Yu harryyu1994@gmail.com

[0xdaryl]

@genie-omr build all

[0xdaryl]

Is this a WIP because it's not ready for review, or because you want to hold off merging until your downstream change is ready to merge? If it's the latter, I'll review.

[harryyu1994]

Hi Daryl, thanks for looking at this. It's the latter, please review for me.

[harryyu1994]

No need to review for now, I found a bug with my code changes

[harryyu1994]

This code change is ready for review and merging.

[0dvictor]

LGTM

[mstoodle]

A nice bit of cleanup, but I'd like to see it taken just a little bit further and the commits structured better.
Thanks @harryyu1994 !

[mstoodle]

bah, i only meant to initiate tests on x platforms :( . oh well, it will be over-the-top tested...

[harryyu1994]

Hi Mark, @mstoodle thanks for the review, appreciate your help! I would like to hold off merging until I verify that the downstream change(openj9) works as intended. I will let you know.

[mstoodle]

fair enough! i marked the dependent PR in Openj9 as depending on this one.

[harryyu1994]

Just got back to this. Removing the J9_PROJECT_SPECIFIC guard in OMR::X86::queryX86TargetCPUID() is causing the OpenJ9 build to fail, so I'm planning to leave it for now. I've ran sanity tests with the remaining two commits along with the downstream changes and they seem to be passing. @mstoodle please help me review and merge when you have time. Downstream change is also ready for review. Thanks in advance!

[mstoodle]

@harryyu1994 can you be more specific about how the OpenJ9 build failed?

[harryyu1994]

@mstoodle When we execute the JVM, we get JVMJ9VM011W Unable to load j9jit29: .../sdk/xa6490/lib/amd64/default/libj9jit29.so: undefined symbol: _ZN2TR9JitConfig8instanceEv -----> probably has something to do with auto jitConfig = TR::JitConfig::instance(); in OMR::X86::queryX86TargetCPUID().

[mstoodle]

@harryyu1994 I actually wasn't suggesting to remove the #ifdef, I was proposing to move the #ifdef controlled code into the J9 specific extension of this class (which already exists): i.e. put this #ifdef'd code into J9::X86::CPU::queryX86TargetCPUID() in the openj9 project and remove it from OMR.

Does that make sense? Not really.

But we could introduce an empty "return false" implementation of TR::JitConfig::instance() into OpenJ9 that would allow us to remove this #ifdef structure. I have to say that the else branch of this code looks pretty weird to me (why not just use jitGetCPUID() even if there isn't a jitconfig?)

[harryyu1994]

@mstoodle Sorry I didn't quite get it. Here are the two versions of queryX86TargetCPUID(), you are saying we should move the code in OMR::queryX86TargetCPUID() to J9::queryX86TargetCPUID(), then what is OMR::queryX86TargetCPUID() left with? Could you please elaborate more on this?

OMR::X86::CPU::queryX86TargetCPUID()
    {
 #ifdef J9_PROJECT_SPECIFIC
    return NULL;
 #else
    static TR_X86CPUIDBuffer *buf = NULL;
    auto jitConfig = TR::JitConfig::instance();
 
    if (jitConfig && jitConfig->getProcessorInfo() == NULL)
       {
       buf = (TR_X86CPUIDBuffer *) malloc(sizeof(TR_X86CPUIDBuffer));
       if (!buf)
          return 0;
       jitGetCPUID(buf);
       jitConfig->setProcessorInfo(buf);
       }
    else
       {
       if (!buf)
          {
          if (jitConfig && jitConfig->getProcessorInfo())
             {
             buf = (TR_X86CPUIDBuffer *)jitConfig->getProcessorInfo();
             }
          else
             {
             buf = (TR_X86CPUIDBuffer *) malloc(sizeof(TR_X86CPUIDBuffer));
 
             if (!buf)
                memcpy(buf->_vendorId, "Poughkeepsie", 12); // 12 character dummy string (NIL term not used)
 
             buf->_processorSignature = 0;
             buf->_brandIdEtc = 0;
             buf->_featureFlags = 0x00000000;
             buf->_cacheDescription.l1instr = 0;
             buf->_cacheDescription.l1data  = 0;
             buf->_cacheDescription.l2      = 0;
             buf->_cacheDescription.l3      = 0;
             }
          }
       }
 
    return buf;
 #endif
    }

J9::X86::CPU::queryX86TargetCPUID() {
		static TR_X86CPUIDBuffer buf = { {'U','n','k','n','o','w','n','B','r','a','n','d'} };
		jitGetCPUID(&buf);
		return &buf;
}

To me, the big chunk of code in OMR::queryX86TargetCPUID() isn't for OpenJ9, and only return NULL is, not sure what is there to move.

[harryyu1994]

@mstoodle Hi Mark, this PR is ready for another review, thanks!

[mstoodle]

These are the changes I was looking for. Thanks, @harryyu1994 !

[0xdaryl]

@mstoodle , I think this is ready to merge.

[charliegracie]

ping @mstoodle

[harryyu1994]

Allow me to rebase and run tests against it before merging.

[harryyu1994]

I think this PR will break a downstream project (OpenJ9), I will work on getting that OpenJ9 PR approved today/tonight.

[mstoodle]

For the record, I believe that's this one: eclipse-openj9/openj9#1613 right @harryyu1994 ?

[harryyu1994]

Will remove WIP tag once eclipse-openj9/openj9#1613 is ready. Currently eclipse-openj9/openj9#1613 needs another PR to be merged.

[Leonardo2718]

@genie-omr build all

@mstoodle ping

[mstoodle]

For completeness: genie-omr build all

The openj9 PR that I suspect is co-dependent on this one cannot be merged right now due to (I believe) unrelated problems in the PR builds. Hopefully those issues will get cleared up soon...

[0xdaryl]

@mstoodle, I believe both the downstream PR and this PR are ready to merge now.

[mstoodle]

OpenJ9 has asked that we hold off on this co-dependent merge until their release is closer to closed away (even though they've already split the release branch). Anyway, that request translated to "please hold off until Monday". Since this PR isn't urgent, I'm accepting the request and will revisit this PR on Monday.

[mstoodle]

Merging now along with dependent OpenJ9 PR