RFR 8239090: Improve CPU feature support in VM_version

[Igor Veresov]

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> 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
>> 
> 
> 
>