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

[Volker Simonis]

Hi Martin,

change looks good!
Thanks for fixing this so fast.

Regards,
Volker


On Wed, Jul 5, 2017 at 5:02 PM, Doerr, Martin <martin.doerr at sap.com> 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.
>
> 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.
>
> 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
>>>>