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

[Niklas Radomski]

On Tue, 2 Feb 2021 09:18:10 GMT, Roman Kennke <rkennke at openjdk.org> wrote:

>> Niklas Radomski has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - Fix carg slot offset calculation
>>  - Use a distinct magic number in clobber_carg_stack_slots
>>  - Use enumeration instead of unsigned int
>
> Looks good!

> 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.

My rule of thumb is: If a line fits into the (nowadays standard) 120 characters line limit quite nicely, I usually don't use additional line breaks. If it doesn't, I do what you've just described.

As far as I can tell, I'm not removing any line breaks that are necessary for keeping this limit. Nonetheless, as the variable names are very compact, the rule might not be as applicable as in other codebases. 

IMHO, it's a matter of taste. As you mention it explicitly, I'll go back to the more aggressive approach.

> 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." ?
> 

That comment was removed in dbc18a3 because it really didn't provide any value.
Suggestion looks good, I'll consider it.

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

Yikes.

> 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.)

As I have to edit that file anyway, I'll rename the other locals as well.
Great opportunity to pay off some styling debts...
Using different naming styles in the same function would presumably be much worse.

> Why do you do these register changes here?

`temp1` is used to store the receiver klass and must survive several calls to `MacroAssembler::load_heap_oop` (and thus JRT calls). If it still was a volatile register, we would have to use a higher preservation level; resulting in additional overhead.

As future readers might stumble across this as well, I'll add a comment to make this design choice a little more obvious in that particular case. I also dislike that `tmp1` is still used after the assignment of `temp1` to `temp1_recv_klass`, but I cannot think of a better solution...

The same holds true for all the other register changes.

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

Although those comments may seem subtle, they actually did lead to some serious confusion!
Nonetheless, quite interesting to find such "artifacts of the past" in the codebase. 

> 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.
> 

If I moved R11_klass, I should probably do the same with all the other declarations (if applicable).
So I'll just leave it as it is. 

> 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?

That is a _really_ good point!
I'll change it and keep it in mind for future changes.

Thank you for your review!

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

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