RFR(S): 8183571: PPC64 build broken after 8178499

[Stefan Karlsson]

On 2017-07-05 17:02, Doerr, Martin wrote:
> Hi Stefan,
> 
> thank you very much for your feedback.
> 
> New webrev:
> http://cr.openjdk.java.net/~mdoerr/8183571_fix_PPC64_build/webrev.01/
> 
> Using static const int makes it easier to maintain, so I'm using this, now. The other enum isn't needed any more, so I've removed it.
> 
> I also changed the cast in vm_version as you proposed. It's better to cast only once.

Looks good!

> 
> The 2 functional changes are what I had called "(Contains minor PPC code improvements.)":
> The "case 0" in c1_Runtime1 just fixes a case which is currently unused in openJDK.
> The additional check in vm_version_ppc prevents code for Power8 from incorrently running on Power5. Nothing critical.
> I'll try to find another reviewer for this.

Thanks,
StefanK

> 
> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: Stefan Karlsson [mailto:stefan.karlsson at oracle.com]
> Sent: Mittwoch, 5. Juli 2017 16:28
> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(S): 8183571: PPC64 build broken after 8178499
> 
> Sorry for multiple mails, but I now also see that you added other
> unrelated changes:
> 
> @@ -286,10 +286,11 @@
>        __ ld(R6_ARG4, frame_size_in_bytes + padding + 16, R1_SP);
>        case 2:
>        __ ld(R5_ARG3, frame_size_in_bytes + padding + 8, R1_SP);
>        case 1:
>        __ ld(R4_ARG2, frame_size_in_bytes + padding + 0, R1_SP);
> +    case 0:
>        call_offset = __ call_RT(noreg, noreg, target);
>        break;
>        default: Unimplemented(); break;
>      }
>      OopMapSet* oop_maps = new OopMapSet();
> 
> You can count me as a Reviewer for the align issues caused by 8178499,
> but you need to leave the rest out of this patch if you want to use me
> as a Reviewer.
> 
> StefanK
> 
> On 2017-07-05 16:23, Stefan Karlsson wrote:
>> On 2017-07-05 16:15, Stefan Karlsson wrote:
>>> Hi Marti,
>>>
>>> On 2017-07-05 15:55, Doerr, Martin wrote:
>>>> Hi,
>>>>
>>>> I have fixed the PPC64 build:
>>>> http://cr.openjdk.java.net/~mdoerr/8183571_fix_PPC64_build/webrev.00/
>>>> (Contains minor PPC code improvements.)
>>>>
>>>> Please review.
>>>
>>> Most looks good to me, but I'm not sure why this was needed because of
>>> 8178499:
>>> -  if (has_mfdscr()) {
>>> +  if (PowerArchitecturePPC64 >= 8 && has_mfdscr()) {
>>>
>>> I'm also not sure why you needed this change:
>>> -  (*test)(align_up(mid_of_test_area, 16), (uint64_t)0);
>>> +  (*test)((address)align_up((intptr_t)mid_of_test_area, 16), 0);
>>
>> And now I read the bug report and understand why need to cast here.
>>
>> Another way to write this could probably be:
>> (*test)(align_up((address)mid_of_test_area, 16), 0);
>>
>> Or maybe even stop mixing address and char*, but that might be much
>> larger task.
>>
>> StefanK
>>
>>>
>>> Did you consider changing:
>>>     enum {
>>>       // stack alignment
>>>       alignment_in_bytes = 16,
>>>       // log_2(16*8 bits) = 7.
>>>       log_2_of_alignment_in_bits = 7
>>>     };
>>>
>>> to the more type safe:
>>>       // stack alignment
>>>       static const int alignment_in_bytes = 16;
>>>       // log_2(16*8 bits) = 7.
>>>       static const int log_2_of_alignment_in_bits = 7
>>>
>>> that way you wouldn't have to cast in code like this:
>>> -  const int frame_size_in_bytes  = align_up(register_save_size,
>>> frame::alignment_in_bytes)
>>> +  const int frame_size_in_bytes  = align_up(register_save_size,
>>> (int)frame::alignment_in_bytes)
>>>
>>> StefanK
>>>
>>>>
>>>> Best regards,
>>>> Martin
>>>>