On Fri, 20 Sep 2024 12:31:18 GMT, Roman Kennke <rken...@openjdk.org> wrote:
>> 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 Something like this is what I have in mind. It seems to pass tier1 tests. I still haven't managed to reproduce the path that requires an index register, though. https://github.com/rkennke/jdk/commit/2c4a7877e4ef94017c8155578d8cfc9342441656 ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20677#discussion_r1768816377
