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

Berg, Michael C michael.c.berg at intel.com
Mon Apr 4 20:05:05 UTC 2016 

FYI

-----Original Message-----
From: Berg, Michael C 
Sent: Monday, April 04, 2016 12:42 PM
To: 'Rahul Raghavan' <rahul.v.raghavan at oracle.com>
Subject: RE: RFR(S): 8149488: Incorrect declaration of bitsInByte in regmask.cpp

Looks ok Rahul.

Thanks,
Michael

-----Original Message-----
From: Rahul Raghavan [mailto:rahul.v.raghavan at oracle.com]
Sent: Monday, April 04, 2016 1:09 AM
To: hotspot-compiler-dev at openjdk.java.net
Cc: Dean Long <dean.long at oracle.com>; Berg, Michael C <michael.c.berg at intel.com>; Tobias Hartmann <tobias.hartmann at oracle.com>; Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
Subject: RE: RFR(S): 8149488: Incorrect declaration of bitsInByte in regmask.cpp

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