[PATCH] D100788: [SystemZ] Support i128 inline asm operands

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa marked an inline comment as done.
jonpa added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8568
+          const TargetRegisterClass *RC = nullptr;
+          if (RegVT == MVT::Untyped) {
+            const MachineFunction &MF = DAG.getMachineFunction();
----------------
uweigand wrote:
> jonpa wrote:
> > uweigand wrote:
> > > jonpa wrote:
> > > > uweigand wrote:
> > > > > Instead of recomputing the RC here, can't we get it from the flag word of the matching operand, e.g. using `hasRegClassConstraint(OpFlag, RCID)`?  This would match the way we get the number of registers from the flag word instead of recomputing it as well.
> > > > No - the matching operand may be a specific physreg in which case there is no register class specified (e.g. SystemZ/asm-17.ll).
> > > > 
> > > Hmm, maybe I'm confused but I thought in those cases, we'd still get the RC for that physreg installed into the flags.  Isnt't that exactly what the code in AddInlineAsmOperands does:
> > > 
> > > ```
> > >     const TargetRegisterClass *RC = MRI.getRegClass(Regs.front());
> > >     Flag = InlineAsm::getFlagWordForRegClass(Flag, RC->getID());
> > > 
> > > ```
> > > (Regs.front() should have been set up to the physreg if there's one.)
> > > 
> > That is guarded with a check to only do that for virtual registers...
> Doh, of course, sorry for missing that.
> 
> Still, I think calling getRegForInlineAsmConstraint again here is redundant.  The routine was already called in GetRegistersForValue, resulting in //either// a virtual register, in which case the RC is stored in the Flags, //or else// a physical register **which is guaranteed to be a member of RC** otherwise GetRegistersForValue will fail an assertion.
> 
> So we should check the register that was created for the matching index (which is in 
> `AsmNodeOperands[CurOp+1]` at this point); if it is a virtual register, get the RC from the flags, and if it is a physical register, use the RC this physreg is a member of.  Does that make sense?
Ah, I was not quite aware of the fact that the phys-reg returned would also have a matching RC returned. 

I also then think it would be best to call getRegForInlineAsmConstraint() once so I tried to pass an RC argument to it by reference so that the RC returned could be used in all the following places. The problem with this is however that targets fall back to the default TargetLowering::getRegForInlineAsmConstraint() which may return an unallocatable register class. I then tried demanding an allocatable register class in that method, but then physregs like CC become a problem since they don't have that. So to do this it seems then all targets must take care to return the right RC if there is not an allocatable one for a physreg.

What's more is that this wasn't actually working for an operand where the reg is a specified GR128 phys-reg.  AddInlineAsmOperands will then call getNumRegisters() without an RC (and return 2 for i128). I tried returning just '1' here for a phys-reg, but that failed on other targets. Even so, InstrEmitter then fails to find the RC for the defined register, so  SystemZTargetLowering::getRegClass() would have to return GR128 for MVT::Untyped...

I wonder:

- Do we want to also handle the explicit phys reg operands (at this point)?
- It is quite messy to first call getRegForInlineAsmConstraint() and then recompute the RC after that fact. Would it make sense to try to make TargetLowering::getRegForInlineAsmConstraint() return an allocatable RC and fix target cases of unallocatable physregs that show up in tests, and then just reuse that RC in throughout visitInlineAsm()? Alternatively, try first to return an allocatable RC and if there is none, return an unallocatable one.

If we are just handling virtual registers, maybe we could just get the RC from the flags per your suggestion...


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

https://reviews.llvm.org/D100788