Handle new CPU enum types inside J9CPU.cpp #10390

dchopra001 · 2020-08-14T14:59:04Z

New CPU types were introduced in the OMRProcessArchitecture enum and OpenJ9 must be able to handle these new entries. Additionally, this PR also adds on to the J9::Z::CPU::applyUserOptions() routine by also updating _supportedArch when the CPU level is changed.

Signed-off-by: Dhruv Chopra <Dhruv.C.Chopra@ibm.com>

fjeremic · 2020-08-14T15:19:40Z

Jenkins test sanity zlinux jdk11

fjeremic · 2020-08-14T16:24:19Z

Depends on eclipse/omr#5459. We'll need to wait for a promotion.

dchopra001 · 2020-08-14T16:25:24Z

@fjeremic I think this one depends on eclipse/omr#5459

fjeremic · 2020-08-14T16:29:54Z

Jenkins test sanity zlinux jdk11 depends eclipse/omr#5459

runtime/compiler/z/env/J9CPU.cpp

harryyu1994

Actually I don't think you need the changes in applyUserOptions() at all.

dchopra001 · 2020-08-17T15:42:41Z

I only added the changes in applyUserOptions because I saw a failure in a test where we purposefully disable architectures to test support. I will try to reproduce it again. It looks like, as you are suggesting, the fix to that issue might be something else.

dchopra001 · 2020-08-17T15:47:23Z

Here are a few examples:

java"   -Xdump -Xjit:count=0,disableZ13 -version
Time spent starting: 1 milliseconds
Time spent executing: 390 milliseconds
Test result: FAILED
Output from test:
 [ERR] Assertion failed at /tmp/bld_453611/bld_linux_390-64_cmprssptrs/compiler/../omr/compiler/z/env/OMRCPU.cpp:117: self()->is_at_least_test(p)
 [ERR] 	processor z14(9) failed, _supportedArch z14(8), _processorDescription.processor zEC12(7)

java"   -Xdump -Xjit:count=0,disableZEC12 -version
Time spent starting: 1 milliseconds
Time spent executing: 338 milliseconds
Test result: FAILED
Output from test:
 [ERR] Assertion failed at /tmp/bld_453611/bld_linux_390-64_cmprssptrs/compiler/../omr/compiler/z/env/OMRCPU.cpp:117: self()->is_at_least_test(p)
 [ERR] 	processor z14(9) failed, _supportedArch z14(8), _processorDescription.processor z196(6)

java"   -Xdump -Xjit:count=0,disableZ196 -version
Time spent starting: 2 milliseconds
Time spent executing: 355 milliseconds
Test result: FAILED
Output from test:
 [ERR] Assertion failed at /tmp/bld_453611/bld_linux_390-64_cmprssptrs/compiler/../omr/compiler/z/env/OMRCPU.cpp:117: self()->is_at_least_test(p)
 [ERR] 	processor z14(9) failed, _supportedArch z14(8), _processorDescription.processor z10(5)

In all of these situations _supportedArch looks like it ignored the disableArch option.

harryyu1994 · 2020-08-17T15:58:22Z

If any update needs to be done for _supportedArch, they should be done in this function. Does this function do what is expected?

void
J9::Z::CPU::initializeS390ProcessorFeatures()
   {
   // The following nested if statements cascade so as to have the effect of only enabling the least common denominator
   // of disable CPU architectures. For example if the user specified to disable z13 when running on a z15 machine the
   // logic below will ensure we only reach the `setSupportsArch` call for zEC12.
   if (TR::Compiler->target.cpu.TO_PORTLIB_get390_supportsZ6() &&
         !TR::Options::getCmdLineOptions()->getOption(TR_DisableZ10))
      {
      TR::Compiler->target.cpu.setSupportsArch(TR::CPU::z10);

      if (TR::Compiler->target.cpu.TO_PORTLIB_get390_supportsZGryphon() &&
            !TR::Options::getCmdLineOptions()->getOption(TR_DisableZ196))
         {
         TR::Compiler->target.cpu.setSupportsArch(TR::CPU::z196);

         if (TR::Compiler->target.cpu.TO_PORTLIB_get390_supportsZHelix() &&
               !TR::Options::getCmdLineOptions()->getOption(TR_DisableZEC12))
            {
            TR::Compiler->target.cpu.setSupportsArch(TR::CPU::zEC12);

            if (TR::Compiler->target.cpu.TO_PORTLIB_get390_supportsZ13() &&
                  !TR::Options::getCmdLineOptions()->getOption(TR_DisableZ13))
               {
               TR::Compiler->target.cpu.setSupportsArch(TR::CPU::z13);

               if (TR::Compiler->target.cpu.TO_PORTLIB_get390_supportsZ14() &&
                     !TR::Options::getCmdLineOptions()->getOption(TR_DisableZ14))
                  {
                  TR::Compiler->target.cpu.setSupportsArch(TR::CPU::z14);

                  if (TR::Compiler->target.cpu.TO_PORTLIB_get390_supportsZ15() &&
                        !TR::Options::getCmdLineOptions()->getOption(TR_DisableZ15))
                     {
                     TR::Compiler->target.cpu.setSupportsArch(TR::CPU::z15);

                     if (TR::Compiler->target.cpu.TO_PORTLIB_get390_supportsZNext() &&
                           !TR::Options::getCmdLineOptions()->getOption(TR_DisableZNext))
                        {
                        TR::Compiler->target.cpu.setSupportsArch(TR::CPU::zNext);
                        }
                     }
                  }
               }
            }
         }
      }
....

dchopra001 · 2020-08-17T20:02:11Z

The backtrace for the above assert is as below:

(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x000003fffdcbf37a in __GI_abort () at abort.c:79
#2  0x000003fff77ed6cc in TR::trap() () from /sandbox/dchopra/development/openj9-openjdk-jdk8/build/linux-s390x-normal-server-release/images/j2sdk-image/jre/lib/s390x/compressedrefs/libj9jit29.so
#3  0x000003fff77ed764 in TR::fatal_assertion(char const*, int, char const*, char const*, ...) () from /sandbox/dchopra/development/openj9-openjdk-jdk8/build/linux-s390x-normal-server-release/images/j2sdk-image/jre/lib/s390x/compressedrefs/libj9jit29.so
#4  0x000003fff7cb5d80 in OMR::Z::CPU::isAtLeast(OMRProcessorArchitecture) () from /sandbox/dchopra/development/openj9-openjdk-jdk8/build/linux-s390x-normal-server-release/images/j2sdk-image/jre/lib/s390x/compressedrefs/libj9jit29.so
#5  0x000003fff736c8b4 in TR_J9VM::initializeProcessorType() () from /sandbox/dchopra/development/openj9-openjdk-jdk8/build/linux-s390x-normal-server-release/images/j2sdk-image/jre/lib/s390x/compressedrefs/libj9jit29.so
#6  0x000003fff736c09e in TR_J9VMBase::initializeSystemProperties() () from /sandbox/dchopra/development/openj9-openjdk-jdk8/build/linux-s390x-normal-server-release/images/j2sdk-image/jre/lib/s390x/compressedrefs/libj9jit29.so

Looking at initializeProcessorType:

void
TR_J9VM::initializeProcessorType()
   {
   TR_ASSERT(_compInfo,"compInfo not defined");
   TR::Compiler->target.cpu.applyUserOptions();

   if (TR::Compiler->target.cpu.isZ())
      {
#if defined(TR_HOST_S390)
      initializeS390ProcessorFeatures();

#if defined(J9ZOS390)
      // Cache whether current process is running in Supervisor State (i.e. Control Region of WAS).
      if (!_isPSWInProblemState())
         _compInfo->setIsInZOSSupervisorState();
#endif
#endif

#ifdef TR_TARGET_S390
      // For AOT shared classes cache processor compatibility purposes, the following
      // processor settings should not be modified.
      if (TR::Compiler->target.cpu.isAtLeast(OMR_PROCESSOR_S390_ZNEXT))

So it looks like applyUserOptions, when invoked, should update support for the new implementation. And initializeS390ProcessorFeatures(), when invoked, should update support for the old implementation. And that is what I see when I added some tracing inside initializeS390ProcessorFeatures() as well. So it does look like it's doing the correct thing.

But then when we later call isAtLeast in this routine, the assert fires. So I'm not sure why this happens.

harryyu1994 · 2020-08-17T20:05:57Z

So it looks like applyUserOptions, when invoked, should update support for the new implementation. And initializeS390ProcessorFeatures(), when invoked, should update support for the old implementation. And that is what I see when I added some tracing inside initializeS390ProcessorFeatures() as well. So it does look like it's doing the correct thing.

But then when we later call isAtLeast in this routine, the assert fires. So I'm not sure why this happens.

Just guessing here. You added initialization of _supportedArch in OMR::Z::CPU::CPU(const OMRProcessorDesc& processorDescription). And that constructor does not look at the DisableArch set of options when initializing _supportedArch

harryyu1994 · 2020-08-17T20:08:23Z

Try this and see if the problem goes away:

#if defined(J9VM_OPT_JITSERVER)
   // In JITServer, we would like to use JITClient's processor info for the compilation
   // The following code effectively replaces the cpu with client's cpu through the getProcessorDescription() that has JITServer support
   if (self()->getPersistentInfo()->getRemoteCompilationMode() == JITServer::SERVER)
      {
      OMRProcessorDesc JITClientProcessorDesc = TR::Compiler->target.cpu.getProcessorDescription();
      _target.cpu = TR::CPU(JITClientProcessorDesc);
#if defined(TR_TARGET_S390)
     _target.cpu.initialzeS390ProcessorFeatures();
#endif
      }
   else
#endif /* defined(J9VM_OPT_JITSERVER) */

then remove the initialization of _supportedArch in OMR::Z::CPU::CPU() constructor, just give it a default z9

harryyu1994 · 2020-08-17T20:16:17Z

Sorry for the complications.. They wouldn't be here if we don't still have 2 implementations of processor detection. Hopefully I'll have time soon to get rid of the old implementation.

dchopra001 · 2020-08-17T20:19:54Z

It sounds like that might be the problem because OMR::Z::CPU::detect(OMRPortLibrary * const omrPortLib) also uses that OMR::Z::CPU::CPU(processorDesc) constructor. I'm trying your suggested fix now.

harryyu1994 · 2020-08-17T20:26:14Z

For what it's worth:

detect() + applyUserOptions() will get the new processor type (_processorDescription) to its final form
initializeS390ProcessorFeatures() will get the old processor type (_supportedArch) to its final form

harryyu1994 · 2020-08-17T20:28:42Z

Oh just realized, I don't think the processor detection test applies to JITServer
We shouldn't need _target.cpu.initialzeS390ProcessorFeatures(); for JITServer

dchopra001 · 2020-08-17T20:33:23Z

It looks like your above suggested solution fixed the problem in my java -version test.

I see the following code:

bool
J9::Z::CPU::is_at_least_test(OMRProcessorArchitecture p)
   {
#if defined(J9VM_OPT_JITSERVER)
   if (TR::CompilationInfo::getStream())
      return true;
#endif /* defined(J9VM_OPT_JITSERVER) */

So I think that test will pass regardless of the above change you suggested. But I can still remove the change for JITServer if you prefer.

harryyu1994 · 2020-08-17T20:35:32Z

yeah we should remove the change for JITServer because it doesn't make sense as JITClientProcessorDesc is already initialized by a remote client JVM.

dchopra001 · 2020-08-18T01:15:04Z

I have reverted the changes in applyUserOptions: 86e45fe

fjeremic · 2020-08-19T14:29:16Z

@harryyu1994 is this one ready? Please add an approval review whenever you get a chance.

Jenkins test sanity zlinux jdk8

Handle new CPU enum types in test routine

86e45fe

Signed-off-by: Dhruv Chopra <Dhruv.C.Chopra@ibm.com>

fjeremic self-assigned this Aug 14, 2020

fjeremic approved these changes Aug 14, 2020

View reviewed changes

fjeremic added arch:z comp:jit labels Aug 14, 2020

fjeremic added the depends:omr label Aug 14, 2020

harryyu1994 reviewed Aug 17, 2020

View reviewed changes

runtime/compiler/z/env/J9CPU.cpp Outdated Show resolved Hide resolved

harryyu1994 suggested changes Aug 17, 2020

View reviewed changes

fjeremic mentioned this pull request Aug 17, 2020

Remove definitions/declarations of unsigned Call/Return ILopcodes #10388

Merged

dchopra001 force-pushed the updateJ9CPU branch from 9c9eebb to 86e45fe Compare August 18, 2020 01:14

harryyu1994 approved these changes Aug 19, 2020

View reviewed changes

fjeremic removed the depends:omr label Aug 19, 2020

fjeremic added the depends:omr:breaking label Aug 19, 2020

fjeremic merged commit 92fcbf4 into eclipse-openj9:master Aug 19, 2020

dchopra001 mentioned this pull request Sep 28, 2020

Port JITServer to Z #6914

Closed

32 tasks

dchopra001 deleted the updateJ9CPU branch July 13, 2021 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle new CPU enum types inside J9CPU.cpp #10390

Handle new CPU enum types inside J9CPU.cpp #10390

dchopra001 commented Aug 14, 2020

fjeremic commented Aug 14, 2020

fjeremic commented Aug 14, 2020 •

edited

dchopra001 commented Aug 14, 2020

fjeremic commented Aug 14, 2020

harryyu1994 left a comment

dchopra001 commented Aug 17, 2020

dchopra001 commented Aug 17, 2020 •

edited

harryyu1994 commented Aug 17, 2020

dchopra001 commented Aug 17, 2020

harryyu1994 commented Aug 17, 2020 •

edited

harryyu1994 commented Aug 17, 2020 •

edited

harryyu1994 commented Aug 17, 2020

dchopra001 commented Aug 17, 2020

harryyu1994 commented Aug 17, 2020

harryyu1994 commented Aug 17, 2020 •

edited

dchopra001 commented Aug 17, 2020

harryyu1994 commented Aug 17, 2020 •

edited

dchopra001 commented Aug 18, 2020

fjeremic commented Aug 19, 2020

Handle new CPU enum types inside J9CPU.cpp #10390

Handle new CPU enum types inside J9CPU.cpp #10390

Conversation

dchopra001 commented Aug 14, 2020

fjeremic commented Aug 14, 2020

fjeremic commented Aug 14, 2020 • edited

dchopra001 commented Aug 14, 2020

fjeremic commented Aug 14, 2020

harryyu1994 left a comment

Choose a reason for hiding this comment

dchopra001 commented Aug 17, 2020

dchopra001 commented Aug 17, 2020 • edited

harryyu1994 commented Aug 17, 2020

dchopra001 commented Aug 17, 2020

harryyu1994 commented Aug 17, 2020 • edited

harryyu1994 commented Aug 17, 2020 • edited

harryyu1994 commented Aug 17, 2020

dchopra001 commented Aug 17, 2020

harryyu1994 commented Aug 17, 2020

harryyu1994 commented Aug 17, 2020 • edited

dchopra001 commented Aug 17, 2020

harryyu1994 commented Aug 17, 2020 • edited

dchopra001 commented Aug 18, 2020

fjeremic commented Aug 19, 2020

fjeremic commented Aug 14, 2020 •

edited

dchopra001 commented Aug 17, 2020 •

edited

harryyu1994 commented Aug 17, 2020 •

edited

harryyu1994 commented Aug 17, 2020 •

edited

harryyu1994 commented Aug 17, 2020 •

edited

harryyu1994 commented Aug 17, 2020 •

edited