Override CPU::detect() on Z and X86

[harryyu1994]

On Z, modifications to the processor description are needed
due to command line options and other factors. On X, unused
features are masked out (may come in handy for processor
compatibility tests)

Issue: #4339

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

[harryyu1994]

This PR depends on #4744, #4838, and #4900.

[harryyu1994]

IMO these post initializations should be in a separate member function and not in the constructor nor in the detect(omrPortLib) static function.

My reasonings:

1. If we put the post initialization in the constructor (CPU(const OMRProcessorDesc& processorDescription)), although that's one fewer call but it will make it always modify the processor description user passes in and that's not ideal.
2. I also thought about putting it in the detect(omrPortLib) that we just recently introduced (so let detect() modify the processor description before constructing CPU). But it doesn't work well when we have multiple platforms.
3. So the best option I could think of is an postInitialization() api that the user has the option to either call or not call. And the static polymorphism will help us to find the correct platform to call.. (We will call it in detect())

@fjeremic need your opinions/suggestions on this.

[fjeremic]

But it doesn't work well when we have multiple platforms.

Why is that? One of the benefits of extensible classes is that everything interfaces through TR:: so you can extend static factory functions.

[harryyu1994]

But it doesn't work well when we have multiple platforms.

Why is that? One of the benefits of extensible classes is that everything interfaces through TR:: so you can extend static factory functions.

Could you show an example of how that works? I only know how to extend member functions with the extensible classes.

[fjeremic]

Could you show an example of how that works? I only know how to extend member functions with the extensible classes.

OMRCodeGenerator is an extensible class which has a static method isILOpCodeSupported which is overridden in the Z codegen for example [2]. Now when you want to call the static method you go through the TR:: concrete namespace [3] so this particular call would delegate to the Z version if we are compiling the Z codegen.

[1] https://github.com/eclipse/omr/blob/f955e91fb663683ad0b71cf47e3fc0dcc0871694/compiler/codegen/OMRCodeGenerator.hpp#L496-L503
[2] https://github.com/eclipse/omr/blob/f955e91fb663683ad0b71cf47e3fc0dcc0871694/compiler/z/codegen/OMRCodeGenerator.hpp#L751
[3] https://github.com/eclipse/omr/blob/f955e91fb663683ad0b71cf47e3fc0dcc0871694/compiler/codegen/OMRCodeGenerator_inlines.hpp#L34

[harryyu1994]

I think what you are suggesting is making detect a static method of TR::CPU.
Like this:

namespace OMR
{
namespace Z
{
class CPU : public OMR::CPU
   {
   public:
   static TR::CPU detect(OMRPortLibrary * const omrPortLib);

TR::CPU is the concrete class the detect method is returning, and since it's not a pointer, we need to know its definition, which means we need to do #include "env/CPU.hpp".

#include "env/OMRCPU.hpp"
namespace TR
{
class CPU : public OMR::CPUConnector
   {
   public:
   CPU() : OMR::CPUConnector() {}
   };
}
#endif

and over here we can see that env/CPU.hpp includes env/OMRCPU.hpp
In conclusion, there is a circular dependency there: env/CPU.hpp needs env/OMRCPU.hpp and env/OMRCPU.hpp needs env/CPU.hpp.
@fjeremic Please correct me if I'm wrong.

[fjeremic]

I think what you are suggesting is making detect a static method of TR::CPU.

That's right.

Wouldn't it work in the same way it does for the current use in #4744? We forward declare the type:
https://github.com/eclipse/omr/pull/4744/files#diff-736aa9f113bc71c7d49ec958c0f87f69L36

[harryyu1994]

I think what you are suggesting is making detect a static method of TR::CPU.

That's right.

Wouldn't it work in the same way it does for the current use in #4744? We forward declare the type:
https://github.com/eclipse/omr/pull/4744/files#diff-736aa9f113bc71c7d49ec958c0f87f69L36

Okay cool. I failed to realize forward declaration still works in this scenario. No more confusions now.

[harryyu1994]

Depends on #4900

[fjeremic]

@genie-omr build all

[harryyu1994]

Corrected a missing '&' in omrsysinfo_processor_set_feature(featureMasks, enabledFeatures[i], TRUE);

[fjeremic]

@genie-omr build all

[harryyu1994]

Build failed due to yet another typo :(. I'm going to wait till it passes ci before requesting for another review this time.

[fjeremic]

@genie-omr build all

[harryyu1994]

Looks to me that all tests passed ("100% tests passed, 0 tests failed out of 27") and it was a test infrastructure failure ("Remote call on omr_ci_1 failed").

12:53:14  27: �[0;32m[----------] �[m3 tests from Special/PPCFenceEncodingTest (1 ms total)
12:53:14  27: 
12:53:14  27: �[0;32m[==========] �[m4019 tests from 75 test cases ran. (1155 ms total)
12:53:14  27: �[0;32m[  PASSED  ] �[m4019 tests.
12:53:14  27: �[0;32m[  ALL TESTS PASSED  ] 
12:53:14  27: �[m
12:53:14  27/27 Test #27: compunittest ......................   Passed    1.54 sec
12:53:14  
12:53:14  100% tests passed, 0 tests failed out of 27
12:53:14  
12:53:14  Total Test time (real) = 294.26 sec
[Pipeline] junit
12:53:15  Recording test results
12:53:16  Remote call on omr_ci_1 failed
[Pipeline] }
[Pipeline] // dir
[Pipeline] }
[Pipeline] // timestamps
[Pipeline] }
[Pipeline] // stage
[Pipeline] stage
[Pipeline] { (Declarative: Post Actions)
[Pipeline] echo
Cleanup workspace
[Pipeline] deleteDir
[Pipeline] }
[Pipeline] // stage
[Pipeline] }
[Pipeline] // withEnv
[Pipeline] }
[Pipeline] // withEnv
[Pipeline] }
[Pipeline] // node
[Pipeline] End of Pipeline
Setting status of 607bb5f15b3671123e77ff03e902de3f0abc18cc to FAILURE with url https://ci.eclipse.org/omr/job/PullRequest-aix_ppc-64/2011/ and message: 'Build finished. '
Using context: continuous-integration/eclipse-omr/pr/aix_ppc-64
java.lang.InterruptedException
	at java.lang.Object.wait(Native Method)
	at java.lang.Object.wait(Object.java:218)
	at hudson.remoting.Request.call(Request.java:177)
	at hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:286)

[fjeremic]

@genie-omr build aix