RFR: 8260368: [PPC64] GC interface needs enhancement to support GCs with load barriers

[Lindenmaier, Goetz]

Hi Niklas, 

I had a look at your change. 
Looks good to me, I only have some minor comments,
See below. 

Best regards,
  Goetz

c1_LIRAssembler_ppc.cpp
ok

g1BarrierSetAssembler_ppc.cpp
ok

You remove a row of line breaks manking them quit long.
In other files, you break the lines very nicely, 
with register arguments on one line, other args in 
another line etc.

g1BarrierSetAssembler_ppc.hpp
ok

barrierSetAssembler_ppc.cpp
ok

barrierSetAssembler_ppc.hpp
ok

cardTableBarrierSetAssembler_ppc.cpp/hpp
ok

modRefBarrierSetAssembler_ppc.cpp/hpp
ok

interp_masm_ppc.hpp
ok

interp_masm_ppc_64.cpp

Please add a comment to load_resolved_reference_at_index()
that the index register content is destroyed.

macroAssembler_ppc.hpp:

RuntimeInvocationPreservationLevel
Document what it is good for, not why you use what C++ construct.
Maybe "Indicate which registers must be preserved when calling into the runtime." ?

+  // This is especially useful for making calls to the JRT in places in which this haven't been done before;
haven't --> hasn't


macroAssembler_ppc.inline.hpp
ok

methodHandles_ppc.cpp

As described below, I think it would be nice if you name 
+  Register temp1      = R30;
R30_temp1.
(This holds also for argbase and param_size, but no need to 
introduce additional changes.)

Why do you do these register changes here?

Some nice cleanups.  The comments with the O5 register etc
might stem from the sparc port ;)


sharedRuntime_ppc.cpp
ok

stubGenerator_ppc.cpp

+      Register tmp1 = R12_tmp, tmp2 = R11_klass;
I think the register rename is pointless. 
I would just use R12_tmp and R11_scratch1.

You could move the definition of R11_klass
below this point, but the list of all regs 
used at the beginning gives a good overview
of used regs, too.


templateInterpreterGenerator_ppc.cpp
ok

templateTable_ppc_64.cpp
+  const Register tmp1 = R11_scratch1,
+                 tmp2 = R12_scratch2;
Why not use R11_scratch1 directly? The name basically 
has the same meaning as tmp.  The advantage is that
the register number is mentioned in the name, which 
makes debugging register problems more easier.

Look at this artificial example: 

__ load_heap_oop(dst, offset, base, tmp1, tmp2, ...
looks good.
__ load_heap_oop(R11_dst, R12_offset, R12_base, tmp1, tmp2,
makes me suspicious: how can offset and base be the same
register?

Further, if reading assembly, it is easier to match
emitting C++ code and the assembly if the register numbers
are mentioned in the variable names.

The existing code was pointless, too:
-  const Register Rscratch = R11_scratch1;
I would think this stems from all the renamings we had
to do in the code when we contributed it to openjdk.





-----Original Message-----
From: hotspot-dev <hotspot-dev-retn at openjdk.java.net> On Behalf Of Niklas Radomski
Sent: Montag, 1. Februar 2021 19:56
To: hotspot-dev at openjdk.java.net
Subject: RFR: 8260368: [PPC64] GC interface needs enhancement to support GCs with load barriers

At present, the `needs_frame` flag is used on the ppc platform to determine whether gc barriers must emit a new stack frame (and save the link register, for that matter) or not. With the introduction of load reference barriers, however, this mechansim is no longer sufficient. This holds especially true for compiler stubs as those make heavy use of volatile registers.

To mitigate this, this patch replaces the `needs_frame` flag with a simple enumeration. As the enumerators are incremental, handling the different "register preservation needs" in the actual gc barrier implementations is comparatively (pun intended) easy. 

_This is a preparational change for the ShenandoahGC port to ppc. As such, it may provide some functionality this version doesn't make use of, but that is required for the upcoming change. This way, the scope of the upcoming change is limited to GC-specific functionality; making its review a little easier._

_For the same reason, this patch also introduces patching support for `LIR_Assembler::leal`._

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

Commit messages:
 - 8260368: Enhance gc interface for advanced gc barriers

Changes: https://git.openjdk.java.net/jdk/pull/2302/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2302&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8260368
  Stats: 306 lines in 19 files changed: 120 ins; 20 del; 166 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2302.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2302/head:pull/2302

PR: https://git.openjdk.java.net/jdk/pull/2302