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

[Doerr, Martin]

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
>>>