Replace CPU APIs on Power #5197

harryyu1994 · 2020-05-14T21:41:19Z

Issue: #4339

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

compiler/env/OMRCPU.cpp

compiler/env/OMRCPU.hpp

gita-omr · 2020-05-15T23:37:09Z

@zl-wang @aviansie-ben fyi

harryyu1994 · 2020-05-19T15:55:45Z

Will squash the new commits once my local tests pass.

compiler/p/env/OMRCPU.cpp

harryyu1994 · 2020-05-19T20:24:29Z

@mpirvu FYI, processor detection work on Power

zl-wang · 2020-05-20T13:03:22Z

Could you show the detected results from before and after this change? e.g. verbose log on a couple different hardwares (P7/P8/P9, LE/BE) Expected same results: CPU type, feature-set, and endian-ness.

Blind testing is not enough, that is.

compiler/env/OMRCompilerEnv.cpp

harryyu1994 · 2020-05-20T18:53:15Z

Could you show the detected results from before and after this change? e.g. verbose log on a couple different hardwares (P7/P8/P9, LE/BE) Expected same results: CPU type, feature-set, and endian-ness.

Blind testing is not enough, that is.

The way I tested it was to add fatal asserts for every query then launch the build in our pool of power machines. (That's how I did it for all the platforms). The assert is basically old api against new api so it ensures that the new api always return the same result as the old api. If you don't think that's enough testing, then we can do what you suggested but I may need assistance on getting these hardwares.

The test I have currently (as an example)

TR_ASSERT_FATAL((_processorDescription.processor == p) == (self()->id() == self()->get_old_processor_type_from_new_processor_type(p)), "is test %d failed, id() %d, _processorDescription.processor %d", p, self()->id(), _processorDescription.processor);

In this case new api is:

(_processorDescription.processor == p)

old api is:

self()->id() == self()->get_old_processor_type_from_new_processor_type(p)

and this is for cpu.is()

Everytime the new api is invoked, we will run the old api to verify if the new api output is correct.

zl-wang · 2020-05-21T13:09:07Z

we don't have any Atlas systems anywhere. old_processor (the old way) identifying Atlas seems something interestingly broken.

harryyu1994 · 2020-05-21T15:41:47Z

I corrected my comment, not an atlas but an OMR_PROCESSOR_PPC_P6 query which is pretty common in the codebase. The old processor returns 16 which seems to be a bogus value. (TR_s370gp14, 16, z processor somehow).

fjeremic · 2020-05-22T16:53:32Z

@genie-omr build all

harryyu1994 · 2020-05-22T17:28:44Z

Going to check if this change breaks OpenJ9..

harryyu1994 · 2020-06-08T15:14:31Z

This is not safe to merge. As far as I can tell, the necessary changes for POWER10 support that I mentioned in my last review have not been made.

As it stands, P10 support will be enabled by default and the get_old_processor_type_from_new_processor_type method will assert-fail if run on POWER10, since it does not handle that processor type.

This is not true unless I switch the power10 queries to the new api form which I won't do in this PR and I put the reasons in the following paragraph. (The old api's test against cpu.id() so even if we have cpu._processorDescription.processor == OMR_PPC_P10 it's not going to break anything)

TR_EnableExperimentalPower10Support can't be checked that early when detect() is called. We do have similar issues on Z and they were handled in a method called applyUserOptions(). I'm planning to do the same for Power.
applyUserOptions() is called in OpenJ9 and we need to change it to be non Z specific. That will make this change dependent on OpenJ9 and we don't want that circular dependency as the OpenJ9 change already depends on this one.
So by leaving out the power10 queries, we can get this change in. Then I'll fix the applyUserOptions() call in OpenJ9. After all that is done, we can have a new PR to switch the power10 queries to the new form. (There is only 1 instance in OMR)

aviansie-ben

Alright, I'm happy with the current situation for POWER10 here, since a plan has been outlined for how this can be moved forward without breaking anything. Great work!

fjeremic · 2020-06-08T21:53:01Z

@genie-omr build all

harryyu1994 · 2020-06-09T19:09:29Z

All checks passed. I will need to rebase to master (there was an OpenJ9 build break on Z) and verify with OpenJ9 to make sure nothing breaks. Will update when test finishes.

Issue: eclipse-omr#4339 Signed-off-by: Harry Yu <harryyu1994@gmail.com>

harryyu1994 · 2020-06-09T22:27:10Z

OpenJ9 tests passed on all platforms. We are good to go!

fjeremic · 2020-06-10T14:09:34Z

@genie-omr build all

fjeremic · 2020-06-10T14:10:47Z

@harryyu1994 can you confirm eclipse-openj9/openj9#9571 will not break following this merge?

harryyu1994 · 2020-06-10T18:46:23Z

@harryyu1994 can you confirm eclipse/openj9#9571 will not break following this merge?

@fjeremic Just confirmed that eclipse-openj9/openj9#9571 works fine with this change. I assume you were asking this question to make sure no more omr dependencies is needed for eclipse-openj9/openj9#9571.

Summary of changes: - Retire old version cpu detection code on Power - Move `CPU::getProcessorDescription()` to base J9CPU class as the 3 platforms (X, Z, Power) share the same code. There is no need to have 3 copies - Reasons for the removal: - We kept the old version because we wanted to make sure new version and old version give us the same behaviour - Now it's been a few month and we don't see any bugs with the new version there's no reason to keep the old version - Also we want to prevent developers from using the old version. Old design - Initializes the `TR_Processor` struct via j9port library - Initialization occurs in `TR_J9VM::initializeProcessorType()` - obtain cpu information using `CPU::TO_PORTLIB_getJ9ProcessorDesc()` - `TR::Compiler->target.cpu.setProcessor` to set processor - Fields and methods associated with the old design live in `Class CPU` - Common APIs: - `CPU::TO_PORTLIB_getJ9ProcessorDesc()` - `j9sysinfo_processor_has_feature(processorDesc, feature)` (j9port library API) - `CPU::id()` - `CPU::setProcessor(TR_Processor)` New design - Initializes the `OMRProcessorDesc` struct via omrport library - `CPU::detect()` to initialize `OMRProcessorDesc processorDescription` - `CPU::applyUserOptions()` to apply any debug options after initialization - `TR::Compiler->target.cpu` singleton variable that holds the CPU instance - `comp()->target().cpu` per compilation variable which is used for cross-cpu compilation support - For JIT we initialize `comp()->target().cpu` with `TR::Compiler->target.cpu` (Host CPU) - For AOT we initialize `comp()->target().cpu` with `TR::Compiler->relocatableTarget.cpu` (Portable CPU) - Fields and methods associated with the new design live in `Class CPU` - Common APIs: - `CPU::supportsFeature(feature)` - `CPU::isAtLeast(OMRProcessorArchitecture)` - Benefits: - Common to all 3 platforms - Simpler interface - Cross-CPU compilation support (Use one cpu for AOT and another for JIT) - The implementation for the new design is first introduced here eclipse-omr/omr#5197 and eclipse-openj9#9571 issue: eclipse-omr/omr#4339 depends on: eclipse-omr/omr#5547 Signed-off-by: Harry Yu <harryyu1994@gmail.com>

harryyu1994 requested review from 0xdaryl, charliegracie, knn-k and mstoodle as code owners May 14, 2020 21:41

knn-k added arch:power comp:compiler labels May 14, 2020

harryyu1994 requested a review from fjeremic as a code owner May 15, 2020 04:06

harryyu1994 force-pushed the replaceCPUAPI_P branch from c12f923 to 76bfd7b Compare May 15, 2020 04:24

fjeremic reviewed May 15, 2020

View reviewed changes

compiler/env/OMRCPU.cpp Outdated Show resolved Hide resolved

compiler/env/OMRCPU.hpp Outdated Show resolved Hide resolved

harryyu1994 changed the title ~~Replace CPU APIs on Power~~ WIP: Replace CPU APIs on Power May 15, 2020

fjeremic self-assigned this May 16, 2020

zl-wang suggested changes May 19, 2020

View reviewed changes

compiler/p/env/OMRCPU.cpp Outdated Show resolved Hide resolved

compiler/p/env/OMRCPU.cpp Outdated Show resolved Hide resolved

compiler/p/env/OMRCPU.cpp Show resolved Hide resolved

compiler/p/env/OMRCPU.cpp Show resolved Hide resolved

harryyu1994 force-pushed the replaceCPUAPI_P branch from 872aa2d to 9a18900 Compare May 20, 2020 01:24

harryyu1994 changed the title ~~WIP: Replace CPU APIs on Power~~ Replace CPU APIs on Power May 20, 2020

mpirvu reviewed May 20, 2020

View reviewed changes

compiler/env/OMRCompilerEnv.cpp Outdated Show resolved Hide resolved

compiler/env/OMRCompilerEnv.cpp Outdated Show resolved Hide resolved

zl-wang approved these changes May 20, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

fjeremic requested a review from mpirvu May 22, 2020 16:54

This comment has been minimized.

Sign in to view

harryyu1994 force-pushed the replaceCPUAPI_P branch 3 times, most recently from e7da235 to 88f08eb Compare June 8, 2020 15:11

harryyu1994 force-pushed the replaceCPUAPI_P branch 2 times, most recently from 93c0f40 to 82933d6 Compare June 8, 2020 20:58

aviansie-ben approved these changes Jun 8, 2020

View reviewed changes

harryyu1994 mentioned this pull request Jun 8, 2020

Store OMRProcessorDesc in AOT Header eclipse-openj9/openj9#9826

Merged

Replace CPU APIs on Power

4259868

Issue: eclipse-omr#4339 Signed-off-by: Harry Yu <harryyu1994@gmail.com>

harryyu1994 force-pushed the replaceCPUAPI_P branch from 82933d6 to 4259868 Compare June 9, 2020 19:10

harryyu1994 mentioned this pull request Jun 10, 2020

Replace CPU APIs on Power eclipse-openj9/openj9#9571

Merged

fjeremic merged commit 31fae28 into eclipse-omr:master Jun 10, 2020

harryyu1994 deleted the replaceCPUAPI_P branch June 10, 2020 19:03

fjeremic mentioned this pull request Jul 6, 2020

Create cross codegen extensible peephole phase #5235

Merged

harryyu1994 mentioned this pull request Sep 16, 2020

Remove old processor detection code on Power eclipse-openj9/openj9#10594

Merged

harryyu1994 mentioned this pull request Oct 5, 2020

Migrate processor and feature detection from OpenJ9 to OMR #4339

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace CPU APIs on Power #5197

Replace CPU APIs on Power #5197

harryyu1994 commented May 14, 2020

gita-omr commented May 15, 2020

harryyu1994 commented May 19, 2020

harryyu1994 commented May 19, 2020

zl-wang commented May 20, 2020 •

edited

Loading

harryyu1994 commented May 20, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

zl-wang commented May 21, 2020

harryyu1994 commented May 21, 2020

This comment has been minimized.

fjeremic commented May 22, 2020

This comment has been minimized.

harryyu1994 commented May 22, 2020

harryyu1994 commented Jun 8, 2020 •

edited

Loading

aviansie-ben left a comment

fjeremic commented Jun 8, 2020

harryyu1994 commented Jun 9, 2020

harryyu1994 commented Jun 9, 2020

fjeremic commented Jun 10, 2020

fjeremic commented Jun 10, 2020

harryyu1994 commented Jun 10, 2020

Replace CPU APIs on Power #5197

Replace CPU APIs on Power #5197

Conversation

harryyu1994 commented May 14, 2020

gita-omr commented May 15, 2020

harryyu1994 commented May 19, 2020

harryyu1994 commented May 19, 2020

zl-wang commented May 20, 2020 • edited Loading

harryyu1994 commented May 20, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

zl-wang commented May 21, 2020

harryyu1994 commented May 21, 2020

This comment has been minimized.

fjeremic commented May 22, 2020

This comment has been minimized.

harryyu1994 commented May 22, 2020

harryyu1994 commented Jun 8, 2020 • edited Loading

aviansie-ben left a comment

Choose a reason for hiding this comment

fjeremic commented Jun 8, 2020

harryyu1994 commented Jun 9, 2020

harryyu1994 commented Jun 9, 2020

fjeremic commented Jun 10, 2020

fjeremic commented Jun 10, 2020

harryyu1994 commented Jun 10, 2020

zl-wang commented May 20, 2020 •

edited

Loading

harryyu1994 commented May 20, 2020 •

edited

Loading

harryyu1994 commented Jun 8, 2020 •

edited

Loading