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

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Tue Feb 2 14:56:01 UTC 2021



> IMHO, it's a matter of taste. As you mention it explicitly, I'll go back to the
> more aggressive approach.
Thanks, not a game-changer anyways ��.
And now they are really short!

> > 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.
Thanks!
 
> > 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.
Good. 

> > 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.
Thanks, makes sense!

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

Looks good now, reviewed!
And thanks a lot for doing this great work on Shenandoah!

Best regards,
  Goetz.


More information about the hotspot-dev mailing list