RFR(S): 8149488: Incorrect declaration of bitsInByte in regmask.cpp
Rahul Raghavan
rahul.v.raghavan at oracle.com
Thu Apr 7 06:43:42 UTC 2016
> -----Original Message-----
> From: Dean Long > Sent: Tuesday, April 05, 2016 12:05 AM
>
> Looks OK.
Thank you Dean.
>
> 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