Quantcast

RFR: 8160748: Inconsistent types for ideal_reg

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR: 8160748: Inconsistent types for ideal_reg

Kim Barrett
Please review this change to Type's ideal_reg to make its return type
consistent with the closely related Node::ideal_reg().  Where Type
used to be an int, it is now a uint.  This eliminates the implicit
narrowing conversion in the initialization of _type_info that caused
build failure for C++11 (gcc with -std=gnu++11 or (not tested, but
presumed fixed) Visual Studio 2015 or later).

Also fixed a number of places where the result of Node::ideal_reg()
was being treated as int rather than uint.

And since I was in the neighborhood (changing the type of
Type::TypeInfo::ideal_reg), also fixed Type::_type_info[] to be const
as apparently intended but not successfully accomplished.  (The msg
member was writable, forcing allocation of the array to be in writable
memory, rather than read-only.)

CR:
https://bugs.openjdk.java.net/browse/JDK-8160748

Webrev:
http://cr.openjdk.java.net/~kbarrett/8160748/hotspot.00/

Testing:
JPRT, rbt hs-tier2,hs-tier3,hs-tier4-comp,hs-tier5-comp

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8160748: Inconsistent types for ideal_reg

Igor Veresov
Looks good to me.

igor

> On Apr 6, 2017, at 9:48 PM, Kim Barrett <[hidden email]> wrote:
>
> Please review this change to Type's ideal_reg to make its return type
> consistent with the closely related Node::ideal_reg().  Where Type
> used to be an int, it is now a uint.  This eliminates the implicit
> narrowing conversion in the initialization of _type_info that caused
> build failure for C++11 (gcc with -std=gnu++11 or (not tested, but
> presumed fixed) Visual Studio 2015 or later).
>
> Also fixed a number of places where the result of Node::ideal_reg()
> was being treated as int rather than uint.
>
> And since I was in the neighborhood (changing the type of
> Type::TypeInfo::ideal_reg), also fixed Type::_type_info[] to be const
> as apparently intended but not successfully accomplished.  (The msg
> member was writable, forcing allocation of the array to be in writable
> memory, rather than read-only.)
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8160748
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8160748/hotspot.00/
>
> Testing:
> JPRT, rbt hs-tier2,hs-tier3,hs-tier4-comp,hs-tier5-comp
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8160748: Inconsistent types for ideal_reg

Kim Barrett
> On Apr 7, 2017, at 3:21 PM, Igor Veresov <[hidden email]> wrote:
>
> Looks good to me.
>
> igor

Thanks Igor.

>
>> On Apr 6, 2017, at 9:48 PM, Kim Barrett <[hidden email]> wrote:
>>
>> Please review this change to Type's ideal_reg to make its return type
>> consistent with the closely related Node::ideal_reg().  Where Type
>> used to be an int, it is now a uint.  This eliminates the implicit
>> narrowing conversion in the initialization of _type_info that caused
>> build failure for C++11 (gcc with -std=gnu++11 or (not tested, but
>> presumed fixed) Visual Studio 2015 or later).
>>
>> Also fixed a number of places where the result of Node::ideal_reg()
>> was being treated as int rather than uint.
>>
>> And since I was in the neighborhood (changing the type of
>> Type::TypeInfo::ideal_reg), also fixed Type::_type_info[] to be const
>> as apparently intended but not successfully accomplished.  (The msg
>> member was writable, forcing allocation of the array to be in writable
>> memory, rather than read-only.)
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8160748
>>
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8160748/hotspot.00/
>>
>> Testing:
>> JPRT, rbt hs-tier2,hs-tier3,hs-tier4-comp,hs-tier5-comp


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8160748: Inconsistent types for ideal_reg

Vladimir Kozlov
In reply to this post by Igor Veresov
Hi, Kim

Please, also correct vector registers (need to fix *.ad files too):

   // Vector ideal reg
   static const int vector_ideal_reg(int len);
   static const int vector_shift_count_ideal_reg(int len);

Can you do the same for OptoReg::reg2stack()? Or file separate RFE for it.

Thanks,
Vladimir

On 4/7/17 12:21 PM, Igor Veresov wrote:

> Looks good to me.
>
> igor
>
>> On Apr 6, 2017, at 9:48 PM, Kim Barrett <[hidden email]> wrote:
>>
>> Please review this change to Type's ideal_reg to make its return type
>> consistent with the closely related Node::ideal_reg().  Where Type
>> used to be an int, it is now a uint.  This eliminates the implicit
>> narrowing conversion in the initialization of _type_info that caused
>> build failure for C++11 (gcc with -std=gnu++11 or (not tested, but
>> presumed fixed) Visual Studio 2015 or later).
>>
>> Also fixed a number of places where the result of Node::ideal_reg()
>> was being treated as int rather than uint.
>>
>> And since I was in the neighborhood (changing the type of
>> Type::TypeInfo::ideal_reg), also fixed Type::_type_info[] to be const
>> as apparently intended but not successfully accomplished.  (The msg
>> member was writable, forcing allocation of the array to be in writable
>> memory, rather than read-only.)
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8160748
>>
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8160748/hotspot.00/
>>
>> Testing:
>> JPRT, rbt hs-tier2,hs-tier3,hs-tier4-comp,hs-tier5-comp
>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8160748: Inconsistent types for ideal_reg

Kim Barrett
> On Apr 10, 2017, at 3:43 PM, Vladimir Kozlov <[hidden email]> wrote:
>
> Hi, Kim
>
> Please, also correct vector registers (need to fix *.ad files too):
>
>  // Vector ideal reg
>  static const int vector_ideal_reg(int len);
>  static const int vector_shift_count_ideal_reg(int len);

Oops, I missed those.  Testing that change.  I’ll put out a new webrev when that’s done.

> Can you do the same for OptoReg::reg2stack()? Or file separate RFE for it.

I’m not sure what it is you are asking for here.

> Thanks,
> Vladimir
>
> On 4/7/17 12:21 PM, Igor Veresov wrote:
>> Looks good to me.
>>
>> igor
>>
>>> On Apr 6, 2017, at 9:48 PM, Kim Barrett <[hidden email]> wrote:
>>>
>>> Please review this change to Type's ideal_reg to make its return type
>>> consistent with the closely related Node::ideal_reg().  Where Type
>>> used to be an int, it is now a uint.  This eliminates the implicit
>>> narrowing conversion in the initialization of _type_info that caused
>>> build failure for C++11 (gcc with -std=gnu++11 or (not tested, but
>>> presumed fixed) Visual Studio 2015 or later).
>>>
>>> Also fixed a number of places where the result of Node::ideal_reg()
>>> was being treated as int rather than uint.
>>>
>>> And since I was in the neighborhood (changing the type of
>>> Type::TypeInfo::ideal_reg), also fixed Type::_type_info[] to be const
>>> as apparently intended but not successfully accomplished.  (The msg
>>> member was writable, forcing allocation of the array to be in writable
>>> memory, rather than read-only.)
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8160748
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~kbarrett/8160748/hotspot.00/
>>>
>>> Testing:
>>> JPRT, rbt hs-tier2,hs-tier3,hs-tier4-comp,hs-tier5-comp


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8160748: Inconsistent types for ideal_reg

Vladimir Kozlov
On 4/10/17 6:00 PM, Kim Barrett wrote:

>> On Apr 10, 2017, at 3:43 PM, Vladimir Kozlov <[hidden email]> wrote:
>>
>> Hi, Kim
>>
>> Please, also correct vector registers (need to fix *.ad files too):
>>
>>  // Vector ideal reg
>>  static const int vector_ideal_reg(int len);
>>  static const int vector_shift_count_ideal_reg(int len);
>
> Oops, I missed those.  Testing that change.  I’ll put out a new webrev when that’s done.
>
>> Can you do the same for OptoReg::reg2stack()? Or file separate RFE for it.
>
> I’m not sure what it is you are asking for here.

There are useless conversions there too. For example:

   static unsigned int reg2stack( OptoReg::Name r) {

       int stack_slot = reg2stack(n);

But it is a lot more changes since stack2reg(int idx) and others have singed argument. And I don't remember if negative
values are used in such case. That is why I said may be to do it in separate RFE.

Vladimir

>
>> Thanks,
>> Vladimir
>>
>> On 4/7/17 12:21 PM, Igor Veresov wrote:
>>> Looks good to me.
>>>
>>> igor
>>>
>>>> On Apr 6, 2017, at 9:48 PM, Kim Barrett <[hidden email]> wrote:
>>>>
>>>> Please review this change to Type's ideal_reg to make its return type
>>>> consistent with the closely related Node::ideal_reg().  Where Type
>>>> used to be an int, it is now a uint.  This eliminates the implicit
>>>> narrowing conversion in the initialization of _type_info that caused
>>>> build failure for C++11 (gcc with -std=gnu++11 or (not tested, but
>>>> presumed fixed) Visual Studio 2015 or later).
>>>>
>>>> Also fixed a number of places where the result of Node::ideal_reg()
>>>> was being treated as int rather than uint.
>>>>
>>>> And since I was in the neighborhood (changing the type of
>>>> Type::TypeInfo::ideal_reg), also fixed Type::_type_info[] to be const
>>>> as apparently intended but not successfully accomplished.  (The msg
>>>> member was writable, forcing allocation of the array to be in writable
>>>> memory, rather than read-only.)
>>>>
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8160748
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~kbarrett/8160748/hotspot.00/
>>>>
>>>> Testing:
>>>> JPRT, rbt hs-tier2,hs-tier3,hs-tier4-comp,hs-tier5-comp
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8160748: Inconsistent types for ideal_reg

Kim Barrett
In reply to this post by Kim Barrett
> On Apr 10, 2017, at 9:00 PM, Kim Barrett <[hidden email]> wrote:
>
>> On Apr 10, 2017, at 3:43 PM, Vladimir Kozlov <[hidden email]> wrote:
>>
>> Hi, Kim
>>
>> Please, also correct vector registers (need to fix *.ad files too):
>>
>> // Vector ideal reg
>> static const int vector_ideal_reg(int len);
>> static const int vector_shift_count_ideal_reg(int len);
>
> Oops, I missed those.  Testing that change.  I’ll put out a new webrev when that’s done.

Here are the requested additional fixes for vector_ideal_reg and
vector_shift_count_ideal_reg.

full: http://cr.openjdk.java.net/~kbarrett/8160748/hotspot.01/
incr: http://cr.openjdk.java.net/~kbarrett/8160748/hotspot.01.inc/

Note that I've not tested the aarch64, ppc, or s390 changes at all.
They are pretty simple though, so hopefully no problems.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8160748: Inconsistent types for ideal_reg

Kim Barrett
In reply to this post by Vladimir Kozlov
> On Apr 10, 2017, at 9:53 PM, Vladimir Kozlov <[hidden email]> wrote:
>
> On 4/10/17 6:00 PM, Kim Barrett wrote:
>>> On Apr 10, 2017, at 3:43 PM, Vladimir Kozlov <[hidden email]> wrote:
>>>
>>> Hi, Kim
>>>
>>> Please, also correct vector registers (need to fix *.ad files too):
>>>
>>> // Vector ideal reg
>>> static const int vector_ideal_reg(int len);
>>> static const int vector_shift_count_ideal_reg(int len);
>>
>> Oops, I missed those.  Testing that change.  I’ll put out a new webrev when that’s done.
>>
>>> Can you do the same for OptoReg::reg2stack()? Or file separate RFE for it.
>>
>> I’m not sure what it is you are asking for here.
>
> There are useless conversions there too. For example:
>
>  static unsigned int reg2stack( OptoReg::Name r) {
>
>      int stack_slot = reg2stack(n);
>
> But it is a lot more changes since stack2reg(int idx) and others have singed argument. And I don't remember if negative values are used in such case. That is why I said may be to do it in separate RFE.

At a high level, I think this is covered by JDK-8135181, and would be part of the JDK-8177482 subtask.
If you want something more specific than that right now, it might be better for you or some other person
with better understanding of the compiler than I have to put a sensible RFE together.  About all I could
say is that reg2stack returns uint but many callers treat the result as an int.  But maybe that’s all you are
asking for?


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8160748: Inconsistent types for ideal_reg

Vladimir Kozlov
In reply to this post by Kim Barrett
Good.

Thanks,
Vladimir

On 4/11/17 5:42 PM, Kim Barrett wrote:

>> On Apr 10, 2017, at 9:00 PM, Kim Barrett <[hidden email]> wrote:
>>
>>> On Apr 10, 2017, at 3:43 PM, Vladimir Kozlov <[hidden email]> wrote:
>>>
>>> Hi, Kim
>>>
>>> Please, also correct vector registers (need to fix *.ad files too):
>>>
>>> // Vector ideal reg
>>> static const int vector_ideal_reg(int len);
>>> static const int vector_shift_count_ideal_reg(int len);
>>
>> Oops, I missed those.  Testing that change.  I’ll put out a new webrev when that’s done.
>
> Here are the requested additional fixes for vector_ideal_reg and
> vector_shift_count_ideal_reg.
>
> full: http://cr.openjdk.java.net/~kbarrett/8160748/hotspot.01/
> incr: http://cr.openjdk.java.net/~kbarrett/8160748/hotspot.01.inc/
>
> Note that I've not tested the aarch64, ppc, or s390 changes at all.
> They are pretty simple though, so hopefully no problems.
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8160748: Inconsistent types for ideal_reg

Vladimir Kozlov
In reply to this post by Kim Barrett


On 4/11/17 6:03 PM, Kim Barrett wrote:

>> On Apr 10, 2017, at 9:53 PM, Vladimir Kozlov <[hidden email]> wrote:
>>
>> On 4/10/17 6:00 PM, Kim Barrett wrote:
>>>> On Apr 10, 2017, at 3:43 PM, Vladimir Kozlov <[hidden email]> wrote:
>>>>
>>>> Hi, Kim
>>>>
>>>> Please, also correct vector registers (need to fix *.ad files too):
>>>>
>>>> // Vector ideal reg
>>>> static const int vector_ideal_reg(int len);
>>>> static const int vector_shift_count_ideal_reg(int len);
>>>
>>> Oops, I missed those.  Testing that change.  I’ll put out a new webrev when that’s done.
>>>
>>>> Can you do the same for OptoReg::reg2stack()? Or file separate RFE for it.
>>>
>>> I’m not sure what it is you are asking for here.
>>
>> There are useless conversions there too. For example:
>>
>>  static unsigned int reg2stack( OptoReg::Name r) {
>>
>>      int stack_slot = reg2stack(n);
>>
>> But it is a lot more changes since stack2reg(int idx) and others have singed argument. And I don't remember if negative values are used in such case. That is why I said may be to do it in separate RFE.
>
> At a high level, I think this is covered by JDK-8135181, and would be part of the JDK-8177482 subtask.

Good. We will take care about this when work on 8177482.

> If you want something more specific than that right now, it might be better for you or some other person
> with better understanding of the compiler than I have to put a sensible RFE together.  About all I could
> say is that reg2stack returns uint but many callers treat the result as an int.  But maybe that’s all you are
> asking for?

I did not know about 8177482 before. It should cover the issue I mentioned.

Thanks,
Vladimir

>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8160748: Inconsistent types for ideal_reg

Kim Barrett
In reply to this post by Vladimir Kozlov
> On Apr 11, 2017, at 10:38 PM, Vladimir Kozlov <[hidden email]> wrote:
>
> Good.
>
> Thanks,
> Vladimir

Thanks.

> On 4/11/17 5:42 PM, Kim Barrett wrote:
>>> On Apr 10, 2017, at 9:00 PM, Kim Barrett <[hidden email]> wrote:
>>>
>>>> On Apr 10, 2017, at 3:43 PM, Vladimir Kozlov <[hidden email]> wrote:
>>>>
>>>> Hi, Kim
>>>>
>>>> Please, also correct vector registers (need to fix *.ad files too):
>>>>
>>>> // Vector ideal reg
>>>> static const int vector_ideal_reg(int len);
>>>> static const int vector_shift_count_ideal_reg(int len);
>>>
>>> Oops, I missed those.  Testing that change.  I’ll put out a new webrev when that’s done.
>>
>> Here are the requested additional fixes for vector_ideal_reg and
>> vector_shift_count_ideal_reg.
>>
>> full: http://cr.openjdk.java.net/~kbarrett/8160748/hotspot.01/
>> incr: http://cr.openjdk.java.net/~kbarrett/8160748/hotspot.01.inc/
>>
>> Note that I've not tested the aarch64, ppc, or s390 changes at all.
>> They are pretty simple though, so hopefully no problems.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: 8160748: Inconsistent types for ideal_reg

Kim Barrett
In reply to this post by Vladimir Kozlov
> On Apr 11, 2017, at 10:41 PM, Vladimir Kozlov <[hidden email]> wrote:
>
>
>
> On 4/11/17 6:03 PM, Kim Barrett wrote:
>>> On Apr 10, 2017, at 9:53 PM, Vladimir Kozlov <[hidden email]> wrote:
>>>
>>> On 4/10/17 6:00 PM, Kim Barrett wrote:
>>>>> On Apr 10, 2017, at 3:43 PM, Vladimir Kozlov <[hidden email]> wrote:
>>>>>
>>>>> Hi, Kim
>>>>>
>>>>> Please, also correct vector registers (need to fix *.ad files too):
>>>>>
>>>>> // Vector ideal reg
>>>>> static const int vector_ideal_reg(int len);
>>>>> static const int vector_shift_count_ideal_reg(int len);
>>>>
>>>> Oops, I missed those.  Testing that change.  I’ll put out a new webrev when that’s done.
>>>>
>>>>> Can you do the same for OptoReg::reg2stack()? Or file separate RFE for it.
>>>>
>>>> I’m not sure what it is you are asking for here.
>>>
>>> There are useless conversions there too. For example:
>>>
>>> static unsigned int reg2stack( OptoReg::Name r) {
>>>
>>>     int stack_slot = reg2stack(n);
>>>
>>> But it is a lot more changes since stack2reg(int idx) and others have singed argument. And I don't remember if negative values are used in such case. That is why I said may be to do it in separate RFE.
>>
>> At a high level, I think this is covered by JDK-8135181, and would be part of the JDK-8177482 subtask.
>
> Good. We will take care about this when work on 8177482.
>
>> If you want something more specific than that right now, it might be better for you or some other person
>> with better understanding of the compiler than I have to put a sensible RFE together.  About all I could
>> say is that reg2stack returns uint but many callers treat the result as an int.  But maybe that’s all you are
>> asking for?
>
> I did not know about 8177482 before. It should cover the issue I mentioned.
>
> Thanks,
> Vladimir

OK, I’ll leave it at that.

Loading...