RFR: JDK-8202016: Use obj+offset in interpreter array access

[B. Blaser]

On 15 May 2018 at 12:43, B. Blaser <bsrbnd at gmail.com> wrote:
> On 15 May 2018 at 11:26, Roman Kennke <rkennke at redhat.com> wrote:
>> Am 14.05.2018 um 23:47 schrieb B. Blaser:
>>> Hi,
>>>
>>> On 14 May 2018 at 13:19, Andrew Dinn <adinn at redhat.com> wrote:
>>>> On 14/05/18 11:15, Roman Kennke wrote:
>>>>> Am 07.05.2018 um 21:47 schrieb Roman Kennke:
>>>>>> In the TemplateTable::aastore() and
>>>>>> InterpreterMacroAssembler::load_resolved_reference_at_index(), the
>>>>>> element address is flattened, and then passed to the BarrierSetAssembler
>>>>>> for actual access. GCs like Shenandoah need the original obj though.
>>>>>>
>>>>>> The proposed change replaces the flattening with base+index+disp
>>>>>> addressing, and removes the shift and add computations. In the case of
>>>>>> aastore, we need to re-fetch the rcx/index from the stack because it
>>>>>> gets trashed on the way.
>>>>>>
>>>>>> Testing: passed: hotspot/jtreg:tier1
>>>>>>
>>>>>> Can I please get a review?
>>>>>> Thanks, Roman
>>>> Well, that x86 code change looks ok/ish/ -- although the need to reload
>>>> from the stack is a tad disappointing.
>>>
>>> Maybe I'm a bit late, but I think we could use r8 & r9 to avoid some
>>> unnecessary stack access, as next.
>>> What do you think (tier1 seems OK)?
>>>
>> Yes, that is a good idea and I believe it would work. I think it can be
>> improved even further by only moving it out to r8/r9 once, and use those
>> registers for addressing. Do you want to file a followup-issue and do that?
>
> Yes, good suggestion.
> I'll provide an improved patch and create a JBS issue.

I've created https://bugs.openjdk.java.net/browse/JDK-8203286

The following patch uses r9 (=rcx) directly for addressing, but
do_oop_store() still expects value in rax, see [1].

Passed: TEST="jtreg:test/hotspot/jtreg:tier1"

I'd need someone to review & push the fix, would you be available for this?

Thanks,
Bernard

[1] http://hg.openjdk.java.net/jdk/jdk/file/f222eba39694/src/hotspot/cpu/x86/templateTable_x86.cpp#l155


diff -r 24151f48582b src/hotspot/cpu/x86/templateTable_x86.cpp
--- a/src/hotspot/cpu/x86/templateTable_x86.cpp    Mon May 14 21:56:07
2018 +0200
+++ b/src/hotspot/cpu/x86/templateTable_x86.cpp    Wed May 16 12:18:50
2018 +0200
@@ -1098,7 +1098,12 @@
   __ movl(rcx, at_tos_p1()); // index
   __ movptr(rdx, at_tos_p2()); // array

-  Address element_address(rdx, rcx,
+#ifdef _LP64
+  __ movptr(r8, rax);
+  __ movl(r9, rcx);
+#endif
+
+  Address element_address(rdx, LP64_ONLY(r9) NOT_LP64(rcx),
                           UseCompressedOops? Address::times_4 :
Address::times_ptr,
                           arrayOopDesc::base_offset_in_bytes(T_OBJECT));

@@ -1125,8 +1130,12 @@
   __ bind(ok_is_subtype);

   // Get the value we will store
+#ifdef _LP64
+  __ movptr(rax, r8); // do_oop_store() expects value in rax
+#else
   __ movptr(rax, at_tos());
   __ movl(rcx, at_tos_p1()); // index
+#endif
   // Now store using the appropriate barrier
   do_oop_store(_masm, element_address, rax, IN_HEAP_ARRAY);
   __ jmp(done);