RFR(S): 8149488: Incorrect declaration of bitsInByte in regmask.cpp

Dean Long dean.long at oracle.com
Mon Apr 4 18:34:45 UTC 2016 

Looks OK.

dl

On 4/4/2016 1:09 AM, Rahul Raghavan wrote:
> Hi,
>
> Please review the revised fix for JDK- 8149488.
>
> <webrev.02>: http://cr.openjdk.java.net/~rraghavan/8149488/webrev.02/
>
> Based on further checking and thanks to clarifications from Michael,
> it was verified that 8149488 issue can be fixed by just correcting the bitsInByte size to 256 in 'regmask.cpp',
> (and that earlier mentioned case of extending bitsInByte table size to 512, is not required).
>
> Points from Michael for the record - "
>     > I believe Dean is right, I have debugged this and analyzed the usage model,
>     > we never made use of the upper components
>     > and register allocation has been right for VecZ for a good deal of time.
>     >
>     > All we need for a change is,
>     > Regmask.cpp:
>     >
>     > uint RegMask::Size() const {
>     >    extern uint8_t bitsInByte[256];
>     >
>     > A one line change.
>     >
>     > -Michael.
>     >
>     > p.s. I ran this on sde for the new hw and on the hw itself, found all is working with the above change.
>     > I tested SPECjvm2008 which as tons of EVEX code generated in it on SKX
>     > where we make use of VecZ and the upper bank of registers."
>
> So in this revised webrev.02, defined and used BITS_IN_BYTE_ARRAY_SIZE for the array size as 256.
>
> Confirmed no issues with 'JPRT -testset hotspot' run.
>
> Thanks,
> Rahul
>
>> -----Original Message-----
>> From: Dean Long > Sent: Friday, April 01, 2016 12:35 AM
>>
>> Michael, isn't the correct size for this table 256?  I missed how VecZ
>> relates to the table size.
>>
>> dl
>>
>> On 3/31/2016 9:58 AM, Berg, Michael C wrote:
>>> Up until now we have gotten along with the size constraint only.
>>> Let us have both the size and the table though for completeness.
>>> I think we can leave the name though.
>>>
>>> -Michael
>>>
>>> -----Original Message-----
>>> From: Rahul Raghavan [mailto:rahul.v.raghavan at oracle.com]
>>> Sent: Thursday, March 31, 2016 9:18 AM
>>> To: Dean Long <dean.long at oracle.com>; hotspot-compiler-dev at openjdk.java.net; Berg, Michael C <michael.c.berg at intel.com>
>>> Subject: RE: RFR(S): 8149488: Incorrect declaration of bitsInByte in regmask.cpp
>>>
>>> Hi Michael,
>>>
>>> With respect to below thread, request help with some questions.
>>> Understood that the bitsInByte[] lookup table tells the number of bits set for the looked-up number and is useful in VectorSet Size.
>>> Also comment got was for requirement to extend bitsInByte table to 512 size, for consistent mapping for VecZ register also, on
>> targets that support it.
>>> But currently for bitsInByte used in VectorSet::Size(), RegMask::Size(), only the original 256 size is sufficient or useful here.
>>> Could not find usage of extended set of values in bitsInByte table! Is it for something to be used in future?
>>>
>>> So for now it seems following original fix for 8149488 - only correcting the size at RegMask::Size(), seems to be okay?
>>> (without extending current bitsInByte array contents) (Anyhow at present values above 0xFF is never indexed for bitsInByte in
>> RegMask::Size())
>>>            ----- src/share/vm/libadt/vectset.hpp
>>>            +#define BITS_IN_BYTE_ARRAY_SIZE 256
>>>            +
>>>
>>>            ----- src/share/vm/opto/regmask.cpp
>>>            -  extern uint8_t bitsInByte[512];
>>>            +  extern uint8_t bitsInByte[BITS_IN_BYTE_ARRAY_SIZE];
>>>
>>>            ----- src/share/vm/libadt/vectset.cpp
>>>            -uint8_t bitsInByte[256] = {
>>>            +uint8_t bitsInByte[BITS_IN_BYTE_ARRAY_SIZE] = {
>>>
>>> I can send revised webrev for above if all okay. Please tell me if I am missing something.
>>>
>>>
>>> OR else if extending bitsInByte array size and entries indeed is the correct fix, the array name also should be changed, right ?
>>> (to maybe 'bitCountArray[BIT_COUNT_ARRAY_SIZE]')
>>>
>>> Thanks,
>>> Rahul
>>>
>>>> -----Original Message-----
>>>> From: Rahul Raghavan > Sent: Thursday, March 31, 2016 6:49 PM
>>>>
>>>>> -----Original Message-----
>>>>> From: Dean Long > Sent: Thursday, March 31, 2016 1:05 AM
>>>>>
>>>>> When do we access elements 256 .. 511?  Wouldn't that mean we have
>>>>> 9-bit bytes?
>>>> Got your point Dean, Thanks.
>>>> I too got some questions here now; will check and reply soon.
>>>>
>>>> -Rahul
>>>>
>>>>> dl
>>>>>
>>>>> On 3/30/2016 12:19 PM, Rahul Raghavan wrote:
>>>>>> Hi,
>>>>>>
>>>>>> With respect to below email thread, request help to review revised webrev.01 for 8149488.
>>>>>>
>>>>>> <bug>: https://bugs.openjdk.java.net/browse/JDK-8149488
>>>>>> <webrev.01>:
>>>>>> http://cr.openjdk.java.net/~rraghavan/8149488/webrev.01/
>>>>>>
>>>>>> Now as required, fixed the issue by extending bitsInByte array size from 256 to 512.
>>>>>> Defined and used BITS_IN_BYTE_ARRAY_SIZE for the array size as 512.
>>>>>> Confirmed no issues with 'JPRT -testset hotspot' run.
>>>>>>
>>>>>> Thanks,
>>>>>> Rahul
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Berg, Michael C [mailto:michael.c.berg at intel.com] > Sent:
>>>>>>> Thursday, February 11, 2016 11:32 PM > To: hotspot-compiler-
>>>>> dev at openjdk.java.net
>>>>>>> Should we not extend:
>>>>>>>
>>>>>>> This does not match the actual definition in share/vm/libadt/vectset.cpp:
>>>>>>>     uint8_t bitsInByte[256] = { // ...
>>>>>>>
>>>>>>> to 512
>>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Berg, Michael C > Sent: Thursday, February 11, 2016 9:57 AM > To: 'Vladimir Ivanov'
>>>>>>>
>>>>>>> So how do we intend to map a VecZ register without 512 bits?
>>>>>>>
>>>>>>> -Michael
>>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: hotspot-compiler-dev
>>>>>>> [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf
>>>>>>> Of Vladimir Ivanov
>>>>>>> Sent: Thursday, February 11, 2016 4:54 AM > To: Rahul Raghavan;
>>>>>>> hotspot-compiler-dev at openjdk.java.net
>>>>>>>
>>>>>>> Rahul,
>>>>>>>
>>>>>>> Can we define a constant instead and use it in both places?
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Vladimir Ivanov
>>>>>>>
>>>>>>> On 2/11/16 10:42 AM, Rahul Raghavan wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Please review the patch for JDK- 8149488.
>>>>>>>>
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8149488
>>>>>>>> Webrev: http://cr.openjdk.java.net/~thartmann/8149488/webrev.00/
>>>>>>>>
>>>>>>>> Corrected the bitsInByte array size in declaration.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Rahul
>>>>>>>>

More information about the hotspot-compiler-dev mailing list