[PATCH] D75014: [InstrEmitter, SystemZ] Copy Access registers with the correct register class.

Mon Mar 2 07:24:10 PST 2020

uweigand accepted this revision.
uweigand added a comment.
This revision is now accepted and ready to land.

In D75014#1898783 <https://reviews.llvm.org/D75014#1898783>, @jonpa wrote:

> In D75014#1897768 <https://reviews.llvm.org/D75014#1897768>, @uweigand wrote:
>
> > > It seems safest to build the target instructions compared to just constrain the virtual register class of the register of the COPY.
> >
> > I'm not sure I understand this: can you explain what problem you see with constraining the register class?
>
>
> I remember seeing that the register allocator would create a new virtual register and give it the register class based on calling MI->getRegClassConstraint() (or TII->getRegClass() directly). So in theory, it seems that if there is no MCInstrDesc anywhere that demands a particular register class, regalloc might feel free to take the optimal one (GRX32). I am not sure this is needed, but there is no mechanism that I know of that would constrain a *COPY* register regclass, although it may be that a COPY of a physreg into a virtreg is left alone.
>
> Maybe someone could instead confirm that physreg copies do not get their virtreg regclasses changed ever. Maybe that is obvious and I just wasn't aware. Or, if there is no guarantee for this, perhaps a target hook like I suggested (getPhysRegCopyRegClass()) would be a better solution after all, since that would also then be an error if regalloc broke that.

Hmm, I see.  I would expect that the one virtreg that was constrained will certainly keep its register class.  However, I guess you're right that regalloc may create *new* virtregs (e.g. due to live range splitting etc.).  I'm not sure either how the case where the virtreg is a COPY of a phyregs will be handled in that case.  Thanks for pointing this out.

>> Also, if you do directly emit the EAR etc., I'm wondering why you still keep the COPY in as well?
> 
> I figured that the coalescer should typically remove it. And I wasn't sure if there could ever be a problem with any other connected virtregs involved having to use a high-register. In that case there would have to be a copy to/from a GRH32.

OK.   Well, it's certainly conservatively correct to do it this way.

Given that the patch as-is looks correct to me, and it does fix an actual bug, I think we should go ahead with it for now.  If we find another solution later (e.g. via common code change), we can always take the SystemZ pass out again.   LGTM.

================
Comment at: llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp:824
+  // Move CC value from a GR32.
   if (DestReg == SystemZ::CC) {
+    unsigned Opcode =
----------------
I'm wondering if we need to check (or at least assert) that SrcReg is even a GRX32 here?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75014/new/

https://reviews.llvm.org/D75014