RFR(S): 8149488: Incorrect declaration of bitsInByte in regmask.cpp
Rahul Raghavan
rahul.v.raghavan at oracle.com
Thu Apr 7 06:43:08 UTC 2016
> -----Original Message-----
> From: Berg, Michael C [mailto:michael.c.berg at intel.com] > Sent: Tuesday, April 05, 2016 1:35 AM
>
> 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.
Thank you Michael.
>
> 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