On Thu, 19 Sep 2024 17:20:36 GMT, Roberto Castañeda Lozano 
<rcastaned...@openjdk.org> wrote:

>> AFAIK, this happens only when using compressed oops with a heap-base in r27. 
>> When running with that setting, we would get addresses like r27[nklass] or 
>> r27[nklass]+offset, both with scale=8. You would need large heaps, perhaps 
>> >4GB, to get this coops setting. The problem with aarch64 is that we can't 
>> have an address like  r27[nklass]+offset, that's why we need to lea the 
>> r27[nklass] part first.
>> Yes, this also happens on x86, but x86 supports  rX[nklass]+offset 
>> addressing.
>
> Thanks @rkennke, I tried running test tiers 1-3 using different compressed 
> OOPs configurations but could not reach this code, unfortunately. Could you 
> provide a reproducer? The reason I am particularly interested is because I'd 
> like to find whether there could be any problematic interaction with C2's 
> implicit null check optimization.

I tried to reproduce for a few hours now using a custom testcase, with no 
success.
I am pretty sure that this can happen, that is why I added this code. 
Originally I had an assert there asserting that index is not used. I do 
remember that this happens very rarely, and I don't remember the exact 
condition. Looking at the possible operands in opclass memory, I think this can 
only happen when we load an nKlass from an address of the form [rX, rY], i.e. 
the address in rX indexed by rY. This is an odd thing to happen for loadNKlass, 
I think, because rY should always be klass_offset_in_bytes. Maybe this is 
possible when we get odd address merges where we get a PhiNode as the 
offset/index? I don't know.
I agree, this *might* lead to surprising problems with implicit null-checking, 
if it is expected that the first instruction in loadNKlass provokes the 
SIGSEGV. A way around this would be to declare an opclass that is a subset of 
'memory' that excludes all operands with index, and match on that. I think this 
would force the lea as a separate instruction and ensure that we never see such 
a thing in loadNKlass. However, I would not feel very confident to do that 
without a reproducer. Let me dig a little further.

For reference, here is my unsuccessful reproducer: 
https://gist.github.com/rkennke/8a57610d74fcde07a9390f268ec35738

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1768538965

Reply via email to