Fix the addition of implicit register operands in incorrect order in SiInstrInfo.

[arphaman]

Summary

This patch fixes a bug in SIInstrInfo where the implicit register operands were added to a machine instruction
in incorrect order - The implicit uses were added before the implicit defs.
I found this bug while trying to enable the implicit register operand verification in machine verifier.

Before:

V_ADD_I32_e32 killed %13, killed %12, implicit %exec, implicit-def %vcc

After:

V_ADD_I32_e32 killed %13, killed %12, implicit-def %vcc, implicit %exec

This patch also makes the method 'addImplicitDefUseOperands' in the machine instruction class public so that we can reuse it in SIInstrInfo.

[arsenm]

lib/Target/AMDGPU/SIInstrInfo.cpp

2600 ↗

(On Diff #31152)

I didn't know about this function. I think it's OK to delete addDescImplicitUseDef and just use Inst->addImplicitDefUseOperands in the one place it is used

[arphaman]

I deleted the method 'SIInstrInfo::addDescImplicitUseDef' and used the method 'addImplicitDefUseOperands' from the machine instruction class directly.

[arsenm]

LGTM

llvm/trunk/include/llvm/CodeGen/MachineInstr.h

   void untieRegOperand(unsigned OpIdx) {
     MachineOperand &MO = getOperand(OpIdx);
     if (MO.isReg() && MO.isTied()) {
       getOperand(findTiedOperandIdx(OpIdx)).TiedTo = 0;
       MO.TiedTo = 0;
     }
   }
 
+  /// Add all implicit def and use operands to this instruction.
+  void addImplicitDefUseOperands(MachineFunction &MF);
 
 private:
   /// If this instruction is embedded into a MachineFunction, return the
   /// MachineRegisterInfo object for the current function, otherwise
   /// return null.
   MachineRegisterInfo *getRegInfo();
 
-  /// Add all implicit def and use operands to this instruction.
-  void addImplicitDefUseOperands(MachineFunction &MF);
-
   /// Unlink all of the register operands in this instruction from their
   /// respective use lists.  This requires that the operands already be on their
   /// use lists.
   void RemoveRegOperandsFromUseLists(MachineRegisterInfo&);
 
   /// Add all of the register operands in this instruction from their
   /// respective use lists.  This requires that the operands not be on their
   /// use lists yet.

llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.h

private:
   void splitScalar64BitBinaryOp(SmallVectorImpl<MachineInstr *> &Worklist,
                                 MachineInstr *Inst, unsigned Opcode) const;
 
   void splitScalar64BitBCNT(SmallVectorImpl<MachineInstr *> &Worklist,
                             MachineInstr *Inst) const;
   void splitScalar64BitBFE(SmallVectorImpl<MachineInstr *> &Worklist,
                            MachineInstr *Inst) const;
 
-  void addDescImplicitUseDef(const MCInstrDesc &Desc, MachineInstr *MI) const;
-
   bool checkInstOffsetsDoNotOverlap(MachineInstr *MIa,
                                     MachineInstr *MIb) const;
 
   unsigned findUsedSGPR(const MachineInstr *MI, int OpIndices[3]) const;
 
 public:
   explicit SIInstrInfo(const AMDGPUSubtarget &st);
 

llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.cpp

if (Opcode == AMDGPU::S_SEXT_I32_I8 || Opcode == AMDGPU::S_SEXT_I32_I16) {
       Inst->addOperand(MachineOperand::CreateImm(Size));
 
     } else if (Opcode == AMDGPU::S_BCNT1_I32_B32) {
       // The VALU version adds the second operand to the result, so insert an
       // extra 0 operand.
       Inst->addOperand(MachineOperand::CreateImm(0));
     }
 
-    addDescImplicitUseDef(NewDesc, Inst);
+    Inst->addImplicitDefUseOperands(*Inst->getParent()->getParent());
 
     if (Opcode == AMDGPU::S_BFE_I32 || Opcode == AMDGPU::S_BFE_U32) {
       const MachineOperand &OffsetWidthOp = Inst->getOperand(2);
       // If we need to move this to VGPRs, we need to unpack the second operand
       // back into the 2 separate ones for bit offset and width.
       assert(OffsetWidthOp.isImm() &&
              "Scalar BFE is only implemented for constant width and offset");
       uint32_t Imm = OffsetWidthOp.getImm();
BuildMI(MBB, MII, DL, get(TargetOpcode::REG_SEQUENCE), ResultReg)
     .addReg(Src.getReg(), 0, AMDGPU::sub0)
     .addImm(AMDGPU::sub0)
     .addReg(TmpReg)
     .addImm(AMDGPU::sub1);
 
   MRI.replaceRegWith(Dest.getReg(), ResultReg);
 }
 
-void SIInstrInfo::addDescImplicitUseDef(const MCInstrDesc &NewDesc,
-                                        MachineInstr *Inst) const {
-  // Add the implict and explicit register definitions.
-  if (NewDesc.ImplicitUses) {
-    for (unsigned i = 0; NewDesc.ImplicitUses[i]; ++i) {
-      unsigned Reg = NewDesc.ImplicitUses[i];
-      Inst->addOperand(MachineOperand::CreateReg(Reg, false, true));
-    }
-  }
-
-  if (NewDesc.ImplicitDefs) {
-    for (unsigned i = 0; NewDesc.ImplicitDefs[i]; ++i) {
-      unsigned Reg = NewDesc.ImplicitDefs[i];
-      Inst->addOperand(MachineOperand::CreateReg(Reg, true, true));
-    }
-  }
-}
-
 unsigned SIInstrInfo::findUsedSGPR(const MachineInstr *MI,
                                    int OpIndices[3]) const {
   const MCInstrDesc &Desc = get(MI->getOpcode());
 
   // Find the one SGPR operand we are allowed to use.
   unsigned SGPRReg = AMDGPU::NoRegister;
 
   // First we need to consider the instruction's operand requirements before

llvm/trunk/test/CodeGen/AMDGPU/si-instr-info-correct-implicit-operands.ll

+; RUN: llc -o /dev/null %s -march=amdgcn -mcpu=verde -verify-machineinstrs -stop-after expand-isel-pseudos 2>&1 | FileCheck %s
+; This test verifies that the instruction selection will add the implicit
+; register operands in the correct order when modifying the opcode of an
+; instruction to V_ADD_I32_e32.
+
+; CHECK: %19 = V_ADD_I32_e32 killed %13, killed %12, implicit-def %vcc, implicit %exec
+
+define void @test(i32 addrspace(1)* %out, i32 addrspace(1)* %in) {
+entry:
+  %b_ptr = getelementptr i32, i32 addrspace(1)* %in, i32 1
+  %a = load i32, i32 addrspace(1)* %in
+  %b = load i32, i32 addrspace(1)* %b_ptr
+  %result = add i32 %a, %b
+  store i32 %result, i32 addrspace(1)* %out
+  ret void
+}