RFR (M) 8146410: Interpreter functions are declared and defined in the wrong files

[Coleen Phillimore]

On 1/5/16 5:28 AM, Andrew Dinn wrote:
> Hi Coleen,
>
> On 04/01/16 22:42, Coleen Phillimore wrote:
>
>> One question for AARCH64 platform in file:
>>
>> http://cr.openjdk.java.net/~coleenp/8146410/src/cpu/aarch64/vm/templateInterpreterGenerator_aarch64.cpp.udiff.html
> Hmm. That's a good question.
>
> The generic code manages relies on the MacroAssembler auxiliary
> bang_stack_with_offset(int offset) called with the relevant offsets. On
> AArch64, that auxiliary method does a store using a register with
> register offset
>
>      mov(rscratch2, -offset);
>      str(zr, Address(sp, rscratch2));
>
> The overriding definition that you have commented out bypasses this
> auxiliary, computing the address in a scratch register then using a
> register direct store
>
>      for (int pages = start_page; pages <= n_shadow_pages ; pages++) {
>        __ sub(rscratch2, sp, pages*page_size);
>        __ str(zr, Address(rscratch2));
>
> I cannot be certain but I recall we found that there was a significant
> performance difference between these two variants -- which means we want
> to retain the definition you have commented out. Andrew Haley (in cc)
> may be able to provide more info here as I believe he added this
> overriding implementation.
>
> I guess a comment on our overriding definition would be in order should
> we ascertain that this was indeed the rationale.

Thank you for the quick answer.  The reason I had trouble with the 
overriding definition in AARCH64 is that I moved 
bang_stack_shadow_pages() to TemplateInterpreterGenerator so the AARCH64 
version would no longer override.  I didn't want to copy the code to the 
other platforms.  The four lines above are something that can easily get 
out of sync and cause hard to find bugs.

     const int page_size = os::vm_page_size();
     const int n_shadow_pages = 
((int)JavaThread::stack_shadow_zone_size()) / page_size;
     const int start_page = native_call ? n_shadow_pages : 1;
     for (int pages = start_page; pages <= n_shadow_pages; pages++) {


Can bang_stack_with_offset on AARCH64 be changed to:

   // Stack overflow checking
   void bang_stack_with_offset(int offset) {
     // stack grows down, caller passes positive offset
     assert(offset > 0, "must bang with positive offset");
+   sub(rscratch2, sp, offset);
+   str(zr, Address(rscratch2));
  -   mov(rscratch2, -offset);
  -   str(zr, Address(sp, rscratch2));
   }


thanks,
Coleen
>
> regards,
>
>
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in UK and Wales under Company Registration No. 3798903
> Directors: Michael Cunningham (US), Michael O'Neill (Ireland), Paul
> Argiry (US)