RFR: 8338658: New Object to ObjectMonitor mapping: s390x implementation

[Lutz Schmidt]

On Wed, 28 Aug 2024 04:26:23 GMT, Amit Kumar <amitkumar at openjdk.org> wrote:

> s390x implementation of [JDK-8315884](https://bugs.openjdk.org/browse/JDK-8315884) New Object to ObjectMonitor mapping; 
> 
> Testing: 
> - tier1-test (fastdebug)
> - tier1-test with UseObjectMonitorTable (fastdebug)
> - tier1-test with UseObjectMonitorTable (release)

I made a few comments you may want to consider.

src/hotspot/cpu/s390/macroAssembler_s390.cpp line 6026:

> 6024:     z_cghi(top, 0);
> 6025:     z_stg(top, Address(basic_lock, BasicObjectLock::lock_offset() + in_ByteSize((BasicLock::object_monitor_cache_offset_in_bytes()))));
> 6026:   }

What is this block good  for? 
Looking at x86, it should store '0' into cache. Instead, it stores whatever top contains. The cgi has no effect.

src/hotspot/cpu/s390/macroAssembler_s390.cpp line 6165:

> 6163:     z_stg(tmp1, Address(box, BasicLock::object_monitor_cache_offset_in_bytes()));
> 6164:   }
> 6165: 

Why not use mvghi here to directly write zero to memory? Prerequisites: displacement must be uimm12.

src/hotspot/cpu/s390/macroAssembler_s390.cpp line 6255:

> 6253:       // check for match.
> 6254:       z_cg(obj, Address(tmp1));
> 6255:       z_bre(monitor_found);

Are we sure there are at least three (this one and two unrolled) non-null cache entries?

src/hotspot/cpu/s390/macroAssembler_s390.cpp line 6261:

> 6259:       z_lg(tmp2, Address(tmp1)); // TODO: top is killed here!!!!
> 6260:       z_ltgr(tmp2, tmp2);
> 6261:       z_brne(loop); // if not EQ to 0, go for another loop

Why not use cghsi (does not kill a register) or ltg at least to check for null?

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

Changes requested by lucy (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20740#pullrequestreview-2276038484
PR Review Comment: https://git.openjdk.org/jdk/pull/20740#discussion_r1741163393
PR Review Comment: https://git.openjdk.org/jdk/pull/20740#discussion_r1741171834
PR Review Comment: https://git.openjdk.org/jdk/pull/20740#discussion_r1741185191
PR Review Comment: https://git.openjdk.org/jdk/pull/20740#discussion_r1741183341