Add recognition of modern Intel processors to port library and compiler #7350
Conversation
0xdaryl
commented
May 28, 2024
0xdaryl
commented
- Improve readability of X86 CPU model and processor macros
- Add recognition of modern Intel processors to port library and compiler
Add underscores for improved readability. Signed-off-by: Daryl Maier <maier@ca.ibm.com>
|
@BradleyWood : please review @vijaysun-omr : FYI |
| TR_ASSERT_FATAL(comp->compileRelocatableCode() || comp->isOutOfProcessCompilation() || comp->compilePortableCode() || comp->target().cpu.is(OMR_PROCESSOR_X86_AMDOPTERON) == cg()->getX86ProcessorInfo().isAMDOpteron(), "isAMDOpteron failed\n"); | ||
| TR_ASSERT_FATAL(comp->compileRelocatableCode() || comp->isOutOfProcessCompilation() || comp->compilePortableCode() || comp->target().cpu.is(OMR_PROCESSOR_X86_INTEL_CORE2) == cg()->getX86ProcessorInfo().isIntelCore2(), "isIntelCore2 failed\n"); | ||
| TR_ASSERT_FATAL(comp->compileRelocatableCode() || comp->isOutOfProcessCompilation() || comp->compilePortableCode() || comp->target().cpu.is(OMR_PROCESSOR_X86_INTEL_NEHALEM) == cg()->getX86ProcessorInfo().isIntelNehalem(), "isIntelNehalem failed\n"); | ||
| TR_ASSERT_FATAL(comp->compileRelocatableCode() || comp->isOutOfProcessCompilation() || comp->compilePortableCode() || comp->target().cpu.is(OMR_PROCESSOR_X86_INTEL_WESTMERE) == cg()->getX86ProcessorInfo().isIntelWestmere(), "isIntelWestmere failed\n"); |
There was a problem hiding this comment.
Are these asserts really necessary?
There was a problem hiding this comment.
I'm not sure, tbh. It has been decades since I looked at this code, but I think there used to be performance considerations for choosing instructions on various processors. But asserts aren't the way to enforce that.
Given what I've come to understand of the CPUID code as part of this PR, it can only be one of those exact processors coming through this path or the assertion will trip. Most of those processors are fairly old, so it seems this path is not taken very frequently (or never at all). (Upon reading the code more closely, I'm not sure this statement is true).
Your question is much broader than what this PR is attempting to address. I'd say it be revisited separately.
And if there is value to keeping these asserts for some reason, we should be able to come up with a more efficient way of expressing these conditions. There is a lot of redundant overhead here.
There was a problem hiding this comment.
Created an issue: #7352
There was a problem hiding this comment.
I believe these asserts were added when, at least in openJ9, we switched to using the portlib for the processor info instead of the X86ProcessorInfo class; I guess it was to ensure that the two sources agreed on all queries.
I doubt these asserts are still needed.
| _processorDescription |= TR_ProcessorIntelIceLake; break; | ||
| case 0x55: // Skylake_X | ||
| case 0x4e: // Skylake_L | ||
| case 0x5e: // Skylake |
There was a problem hiding this comment.
Whats the meaning behind the suffix in your comment?
There was a problem hiding this comment.
I added a comment in my latest forced push. I borrowed the suffix conventions for the processors we recognize from the Linux kernel.
| _processorDescription |= TR_ProcessorIntelSapphireRapids; break; | ||
| case 0x6a: // IceLake_X | ||
| case 0x6c: // IceLake_D | ||
| case 0x7d: // IceLake |
There was a problem hiding this comment.
I could not find 7D documented. I assume its a client processor?
For the client ice lake processors I see
| Processor | Family | Extended Family | Model |
|---|---|---|---|
| Ice Lake Client (U) | 0x6 | 0x7 | 0xe |
| Ice Lake Client (Y) | 0x6 | 0x7 | 0xe |
There was a problem hiding this comment.
Yes, see Vol 4., Table 2-1
| case 0x7d: // IceLake | ||
| case 0x7e: // IceLake_L | ||
| _processorDescription |= TR_ProcessorIntelIceLake; break; | ||
| case 0x55: // Skylake_X |
There was a problem hiding this comment.
I'm not sure how skylake 0x55 will be differentiated from Cooper lake and Cascade lake. The model numbers overlap.
There was a problem hiding this comment.
Both are considered "optimizations" on Skylake. I believe the stepping is what distinguishes them. I didn't include them because I did not find a verifiable stepping value for both, and distinguishing the processor with that much refinement was not my goal (it may be part of your future CPUID refactoring work).
Nevertheless I have since found a stepping value for Cascade Lake, and I modified the PR to support it.
port/common/omrsysinfo_helpers.c
Outdated
| @@ -247,6 +269,7 @@ omrsysinfo_get_x86_description(struct OMRPortLibrary *portLibrary, OMRProcessorD | |||
| char vendor[12]; | |||
| uint32_t familyCode = 0; | |||
| uint32_t processorSignature = 0; | |||
| uint32_t processorStepping = 0; | |||
There was a problem hiding this comment.
Indentation issues here (mixed tabs and spaces)
There was a problem hiding this comment.
Fixed in latest push. Thanks.
* Cascade Lake * Ice Lake * Sapphire Rapids * Emerald Rapids Signed-off-by: Daryl Maier <maier@ca.ibm.com>
|
Jenkins build win,xlinux |
