RFR 8239090: Improve CPU feature support in VM_version
Volker Simonis
volker.simonis at gmail.com
Fri Sep 11 12:00:56 UTC 2020
On Tue, Sep 8, 2020 at 6:41 PM Hohensee, Paul <hohensee at amazon.com> wrote:
>
> Thank you for the review, Igor.
>
> I did indeed define FEATURES_NAMES to be close to the flags enum definition. And, as a macro, it might be useful in some future context. I also defined four CPU_ enum values and strings per line so it’s easy to keep track of the correspondence.
>
> Anyone else up for a review please? I expect I’ll have to turn this into a PR though.
>
Hi Paul,
I like this cleanup and it looks good to me.
I wonder if we shouldn't assert (e.g. in
"VM_Version::get_processor_features()") that the number elements in
"_features_names" equals the number of values in "Feature_Flag".
Unfortunately we can't query the number of elements in an enum in C++,
but we could do something like the following:
enum Feature_Flag : uint64_t {
...
CPU_HV = (1ULL << 46) // Hypervisor instructions
MAX_FEATURE = (1ULL << 46) // Always the same like the largest feature
};
assert(exact_log(MAX_FEATURE) == sizeof(_features_names)/sizeof(char*), ...)
It's just an idea and I leave it up to you if you want to add it to
the change. As I said before, the change looks good to me even without
this extra check.
Best regards,
Volker
> Paul
>
> From: Igor Veresov <igor.veresov at oracle.com>
> Date: Friday, September 4, 2020 at 4:04 PM
> To: "Hohensee, Paul" <hohensee at amazon.com>
> Cc: "hotspot-compiler-dev at openjdk.java.net" <hotspot-compiler-dev at openjdk.java.net>
> Subject: RE: RFR 8239090: Improve CPU feature support in VM_version
>
> This looks good. Did you make FEATURES_NAMES a macro just so that it’s close to the flags enum?
>
> igor
>
>
>
>
>
> On Sep 4, 2020, at 10:39 AM, Hohensee, Paul <hohensee at amazon.com<mailto:hohensee at amazon.com>> wrote:
>
> Slightly adjusted patch.
>
> http://cr.openjdk.java.net/~phh/8239090/webrev.02/
>
> Thanks,
> Paul
>
> On 9/3/20, 3:47 PM, "hotspot-compiler-dev on behalf of Hohensee, Paul" <hotspot-compiler-dev-retn at openjdk.java.net on behalf of hohensee at amazon.com> wrote:
>
> Taking over from Eric...
>
> Thank you for the review, Igor. I took a completely different (and very old approach), however, and defined a method Abstract_VM_Version:: insert_features_names() that iterates over the feature flags set. If a feature bit is on, it appends to an output buffer a corresponding name string from an array indexed by the bit number. I've implemented it only for x86: using the mechanism for other platforms can be follow-on RFEs. I'd greatly appreciate a review.
>
> Webrev: http://cr.openjdk.java.net/~phh/8239090/webrev.00/
>
> To add a feature bit, all one now has to do is add a CPU_ definition and corresponding name string in the FEATURES_NAMES macro.
>
> I've also included a few small changes to the x86 implementation beyond the above.
>
> 1. Unified the previous two bitset definitions into a single Feature_Flag enum and made it a uint64_t.
> 2. supports_tscinv_bit() referenced the CPU_TSCINV bit, which was a bit misleading, so added a new CPU_TSCINV_BIT mask and used it instead.
> 3. Repurposed CPU_TSCINV for supports_tscinv(), which was a "composite" property, but is now computed once in feature_flags().
> 4. Made supports_clflushopt() and supports_clwb() common to both 32 and 64-bit rather than have 32-bit versions that always return 'false'. These bits are never set by the hardware on 32-bit, so no need for separate methods.
> 5. Renamed CPU_HV_PRESENT to CPU_HV to conform with the CPU_ bit naming scheme. "_PRESENT" is redundant and not used for any other CPU_ name, and the feature string uses "hv", not "hv_present". Added CPU_HV to vmStructs_x86.hpp and vmStructs_jvmci.cpp.
>
> Tested using -Xlog:os+cpu on my macbook pro: the same feature string is returned after the patch as before it. Suggestions for how to more thoroughly test the patch are very welcome.
>
> Thanks,
> Paul
>
> On 8/27/20, 6:22 PM, "hotspot-compiler-dev on behalf of Igor Veresov" <hotspot-compiler-dev-retn at openjdk.java.net on behalf of igor.veresov at oracle.com> wrote:
>
> You can actually make a constexpr array of feature objects and then use constexpr function with a loop to look it up. The c++ compiler will generate an O(1) table lookup for it.
> That would be a good way to get rid of the ugly macro (we allow c++14 now).
>
> For example foo() in this example:
>
> enum E { a, b, c };
>
> struct P {
> E _e; // key
> int _v; // value
> constexpr P(E e, int v) : _e(e), _v(v) { }
> };
>
>
> constexpr static P ps[3] = { P(a, 0xdead), P(b, 0xbeef), P(c, 0xf00d)};
>
> constexpr int match(E e) {
> for (const auto& p : ps) {
> if (p._e == e) {
> return p._v;
> }
> }
> return -1;
> }
>
>
> int foo(E e) {
> return match(e);
> }
>
> Will be compiled into:
>
> __Z3foo1E: ## @_Z3foo1E
> .cfi_startproc
> ## %bb.0:
> movl $-1, %eax
> cmpl $2, %edi
> ja LBB0_2
> ## %bb.1:
> pushq %rbp
> .cfi_def_cfa_offset 16
> .cfi_offset %rbp, -16
> movq %rsp, %rbp
> .cfi_def_cfa_register %rbp
> movslq %edi, %rax
> leaq l_switch.table._Z3foo1E(%rip), %rcx
> movq (%rcx,%rax,8), %rax
> movl 4(%rax), %eax
> popq %rbp
> LBB0_2:
> retq
> .cfi_endproc
> ## -- End function
> .section __TEXT,__const
> .p2align 4 ## @_ZL2ps
> __ZL2ps:
> .long 0 ## 0x0
> .long 57005 ## 0xdead
> .long 1 ## 0x1
> .long 48879 ## 0xbeef
> .long 2 ## 0x2
> .long 61453 ## 0xf00d
>
> .section __DATA,__const
> .p2align 3 ## @switch.table._Z3foo1E
> l_switch.table._Z3foo1E:
> .quad __ZL2ps
> .quad __ZL2ps+8
> .quad __ZL2ps+16
>
>
> igor
>
>
>
> On Aug 27, 2020, at 11:08 AM, Eric, Chan <jingxinc at amazon.com> wrote:
>
> Hi,
>
> Requesting review for
>
> Webrev : http://cr.openjdk.java.net/~phh/8239090/webrev.00/
> JBS : https://bugs.openjdk.java.net/browse/JDK-8239090
>
> Yesterday I sent a wrong one, so I send it again,
> I improve the “get_processor_features” method by store every cpu features in an enum array so that we don’t have to count how many “%s” that need to added. I passed the tier1 test successfully.
>
> Regards,
> Eric Chen
>
>
>
More information about the hotspot-compiler-dev
mailing list