[PATCH] D154245: [RISCV] Remove legacy TA/TU pseudo distinction for binary instructions

[Craig Topper via Phabricator via llvm-commits]

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

LGTM



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:2543
                                   .add(MI.getOperand(0))
+                                  .addReg(MI.getOperand(0).getReg(), RegState::Undef)
                                   .add(MI.getOperand(1))
----------------
reames wrote:
> reames wrote:
> > craig.topper wrote:
> > > reames wrote:
> > > > craig.topper wrote:
> > > > > What happens if we don't use undef here?
> > > > If I remember correctly, you get a verifier error due to a use before definition.
> > > Shouldn't we be using the register from Operand 1 not Operand 0?
> > Not unless I'm missing something?
> > 
> > We're forming the non-tied form of the instruction here.  This used to be the unsuffixed (TA) version, and thus the merge was implicitly undef.  With the new representation, we have the policy and merge operands.  We have to explicitly form the undef merge operand.  
> > 
> > Since the operand is undef, it can be any register whose class matches the def, but it conceptually makes the most sense to me to have this be the destination register given that's what it's tied to.  I believe that using 0 or 1 here is NFC, provided it's flagged undef.  If you have a strong preference here, I'm happy to change.
> > 
> Actually, using operand 1 isn't allowed by the verifier.  It's checked in MachineVerifier.cpp like so.
> 
> ```
>     unsigned DefIdx;
>     if (MF->getProperties().hasProperty(
>             MachineFunctionProperties::Property::TiedOpsRewritten) &&
>         MO->isUse() && MI->isRegTiedToDefOperand(MONum, &DefIdx) &&
>         Reg != MI->getOperand(DefIdx).getReg())
>       report("Two-address instruction operands must be identical", MO, MONum);
> 
> ```
Ok I wasn't quite sure what side of the transition out of SSA we're on and what the Two-Address instruction pass will do after this call. 

I think based on what you've said, operand 0 with undef is correct. Thanks.


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

https://reviews.llvm.org/D154245