RFR: 8319801: Recursive lightweight locking: aarch64 implementation [v16]

[Andrew Haley]

On Wed, 14 Feb 2024 08:42:16 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> src/hotspot/cpu/aarch64/c2_MacroAssembler_aarch64.cpp line 416:
>> 
>>> 414: 
>>> 415:     // mark contains the tagged ObjectMonitor*.
>>> 416:     const Register monitor = mark;
>> 
>> I wouldn't do this. Making `monitor` a register alias for `mark` is at best confusing and at worst dangerous. As a general rule, aliased register names have caused many bugs.
>
> I have been trying to think of alternatives which improve this situation. 
> 
> Is it the fact that monitor alias mark, or any of the aliases used for the in-parameters that is confusing?
> If it is the first do you see the following as a solution:
> ```c++
>     const Register monitor = t1;
>     const uintptr_t monitor_tag = markWord::monitor_value;
> 
>     // Untag the monitor.
>     sub(monitor, mark, monitor_tag);
> 
> and for the lock case
> ```c++
>     const Register tagged_monitor = t1;
>     mov(tagged_monitor, mark); 
> 
> where the mov will emit no instruction as they are the same register.
> (These two variants seem somewhat equivalent to me, but it was not clear from your comment if this is the issue)
> 
> If it is the second, I feel like the best compromise would to still alias register via named variables, but give them names that show their origin (so you do not need a language server or scroll/grep to get the contextual information). 
> E.g.
> ```c++
>     const Register t1_monitor = t1;
>     const uintptr_t monitor_tag = markWord::monitor_value;
> 
>     // Untag the monitor.
>     sub(t1_monitor, t1_mark, monitor_tag);
> 
> 
> I would like to hear if I have misunderstood something and / or if you have another proposed solution to this issue.
> 
> This seem to be the final issue blocking integration.

These are both misleading to the reader. Sure, they're cute, but they're dangerous, as has proved in practice several times in the AArch64 port. Writing a `mov` that is known to do nothing is severely misleading. It can be mitigated with a "this mov does nothing" comment.

A prefix naming convention is a significant improvement, . It may also be a good idea to poison the no-longer-valid names (with a `#define`).

Bear in mind that the typical reader is skim-reading this stuff.

I've been trying a better way to do register allocation in hand-coded assembly, where a `RegSet` contains the currently-free registers. At the end of its live range, a value is pushed back into the free set. This avoids most of the pitfalls. (I'm not suggesting that here, it's something to think about for tr future.)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16608#discussion_r1490704403