AMDGPU: Simplify VOP2 operand legalization

[arsenm]

Summary

Checking the legality of src0 is pointless since
it can accept any type of operand.

Also skips the more complex checks for checking
literal operands and constant bus restrictions.
These should not be a concern when performing
legalization.

This gets called quite a few times and the
attempts at commuting are a significant fraction
of the time spent in SIFixSGPRCopies, so it's
somewhat worthwhile to optimize.

[• tstellarAMD]

lib/Target/AMDGPU/SIInstrInfo.cpp

802–805

This isn't true for instructions with implicit defs/uses of VCC.

I prefer the explicit operand checking for legalization rather than make assumptions based on the instruction type, because there are usually one or two corner cases for each instruction type.

[arsenm]

lib/Target/AMDGPU/SIInstrInfo.cpp

802–805

This isn't a concern here because commuteInstruction shouldn't ever see an illegal operand combination / constant bus restriction violation. Part of this set of changes it separating commuting for the common code's purposes (e.g. MachineCSE) and operand legalization. The only times constant bus violations are OK is coming out of isel and as an intermediate step during moveToVALU. This function shouldn't be used for legalization purposes. The constant bus restrictions should only be handled in legalizeOperands, although I need to double check if the VCC check is handled there already. For the implicit VCC cases, we should probably use an operand type that only includes VGPRs + inline immediate for src0/src1.

[• tstellarAMD]

This patch didn't apply cleanly, so I'm not sure if I applied it wrong, but I am getting an assertion failure with this test case: http://people.freedesktop.org/~tstellar/fail.ll

[arsenm]

In D13085#271590, @tstellarAMD wrote:

This patch didn't apply cleanly, so I'm not sure if I applied it wrong, but I am getting an assertion failure with this test case: http://people.freedesktop.org/~tstellar/fail.ll

I don't see the assert when I rebase it. However I do see a lot of verifier errors which are fixed by the next patch, so I split them wrong. I'll just squash this into the other one

lib/Target/AMDGPU/SIInstrInfo.h

public:
   /// instead of MOV.
   void legalizeOpWithMove(MachineInstr *MI, unsigned OpIdx) const;
 
   /// \brief Check if \p MO is a legal operand if it was the \p OpIdx Operand
   /// for \p MI.
   bool isOperandLegal(const MachineInstr *MI, unsigned OpIdx,
                       const MachineOperand *MO = nullptr) const;
 
+  /// \brief Check if \p MO would be a valid operand for the given operand
+  /// definition \p OpInfo. Note this does not attempt to validate constant bus
+  /// restrictions (e.g. literal constant usage).
+  bool isLegalVSrcOperand(const MachineRegisterInfo &MRI,
+                          const MCOperandInfo &OpInfo,
+                          const MachineOperand &MO) const;
+
+  /// \brief Check if \p MO (a register operand) is a legal register for the
+  /// given operand description.
+  bool isLegalRegOperand(const MachineRegisterInfo &MRI,
+                         const MCOperandInfo &OpInfo,
+                         const MachineOperand &MO) const;
+
   /// \brief Fix operands in \p MI to satisfy constant bus requirements.
   void legalizeOperandsVOP3(MachineRegisterInfo &MRI, MachineInstr *MI) const;
 
   /// \brief Legalize all operands in this instruction.  This function may
   /// create new instruction and insert them before \p MI.
   void legalizeOperands(MachineInstr *MI) const;
 
   /// \brief Split an SMRD instruction into two smaller loads of half the

lib/Target/AMDGPU/SIInstrInfo.cpp

MachineInstr *SIInstrInfo::commuteInstruction(MachineInstr *MI,
   MachineOperand &Src0 = MI->getOperand(Src0Idx);
   if (!Src0.isReg())
     return nullptr;
 
   int Src1Idx = AMDGPU::getNamedOperandIdx(MI->getOpcode(),
                                            AMDGPU::OpName::src1);
   MachineOperand &Src1 = MI->getOperand(Src1Idx);
 
-  // Make sure it's legal to commute operands for VOP2.
-  if (isVOP2(MI->getOpcode()) &&
-      (!isOperandLegal(MI, Src0Idx, &Src1) ||
-       !isOperandLegal(MI, Src1Idx, &Src0))) {
+
+  if (isVOP2(MI->getOpcode())) {
+    const MCInstrDesc &InstrDesc = get(MI->getOpcode());
+    // For VOP2 instructions, any operand type is valid to use for src0.  Make
+    // sure we can use the src1 as src0.
+    //
+    // We could be stricter here and only allow commuting if there is a reason
+    // to do so. i.e. if both operands are VGPRs there is no real benefit,
+    // although MachineCSE attempts to find matches by commuting.
+    const MachineRegisterInfo &MRI = MI->getParent()->getParent()->getRegInfo();
+    if (!isLegalRegOperand(MRI, InstrDesc.OpInfo[Src1Idx], Src0))
       return nullptr;
   }
 
   if (!Src1.isReg()) {
     // Allow commuting instructions with Imm operands.
     if (NewMI || !Src1.isImm() ||
         (!isVOP2(MI->getOpcode()) && !isVOP3(MI->getOpcode()))) {
       return nullptr;
     }
-
     // Be sure to copy the source modifiers to the right place.
     if (MachineOperand *Src0Mods
-          = getNamedOperand(*MI, AMDGPU::OpName::src0_modifiers)) {
       MachineOperand *Src1Mods
         = getNamedOperand(*MI, AMDGPU::OpName::src1_modifiers);
 
       int Src0ModsVal = Src0Mods->getImm();
       if (!Src1Mods && Src0ModsVal != 0)
         return nullptr;
 
       // XXX - This assert might be a lie. It might be useful to have a neg

 // Change the order of operands from (0, 1, 2) to (0, 2, 1)
 void SIInstrInfo::swapOperands(MachineBasicBlock::iterator Inst) const {
   assert(Inst->getNumExplicitOperands() == 3);
   MachineOperand Op1 = Inst->getOperand(1);
   Inst->RemoveOperand(1);
   Inst->addOperand(Op1);
 }
 
+bool SIInstrInfo::isLegalRegOperand(const MachineRegisterInfo &MRI,
+                                    const MCOperandInfo &OpInfo,
+                                    const MachineOperand &MO) const {
+  if (!MO.isReg())
+    return false;
+
+  unsigned Reg = MO.getReg();
+  const TargetRegisterClass *RC =
+    TargetRegisterInfo::isVirtualRegister(Reg) ?
+    MRI.getRegClass(Reg) :
+    RI.getPhysRegClass(Reg);
+
+  // In order to be legal, the common sub-class must be equal to the
+  // class of the current operand.  For example:
+  //
+  // v_mov_b32 s0 ; Operand defined as vsrc_32
+  //              ; RI.getCommonSubClass(s0,vsrc_32) = sgpr ; LEGAL
+  //
+  // s_sendmsg 0, s0 ; Operand defined as m0reg
+  //                 ; RI.getCommonSubClass(s0,m0reg) = m0reg ; NOT LEGAL
+
+  return RI.getCommonSubClass(RC, RI.getRegClass(OpInfo.RegClass)) == RC;
+}
+
+bool SIInstrInfo::isLegalVSrcOperand(const MachineRegisterInfo &MRI,
+                                     const MCOperandInfo &OpInfo,
+                                     const MachineOperand &MO) const {
+  if (MO.isReg())
+    return isLegalRegOperand(MRI, OpInfo, MO);
+
+  // Handle non-register types that are treated like immediates.
+  assert(MO.isImm() || MO.isTargetIndex() || MO.isFI());
+  return true;
+}
+
 bool SIInstrInfo::isOperandLegal(const MachineInstr *MI, unsigned OpIdx,
                                  const MachineOperand *MO) const {
   const MachineRegisterInfo &MRI = MI->getParent()->getParent()->getRegInfo();
   const MCInstrDesc &InstDesc = get(MI->getOpcode());
   const MCOperandInfo &OpInfo = InstDesc.OpInfo[OpIdx];
   const TargetRegisterClass *DefinedRC =
       OpInfo.RegClass != -1 ? RI.getRegClass(OpInfo.RegClass) : nullptr;
   if (!MO)
for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
           usesConstantBus(MRI, Op, getOpSize(*MI, i))) {
         return false;
       }
     }
   }
 
   if (MO->isReg()) {
     assert(DefinedRC);
-    const TargetRegisterClass *RC =
-        TargetRegisterInfo::isVirtualRegister(MO->getReg()) ?
-            MRI.getRegClass(MO->getReg()) :
-            RI.getPhysRegClass(MO->getReg());
-
-    // In order to be legal, the common sub-class must be equal to the
-    // class of the current operand.  For example:
-    //
-    // v_mov_b32 s0 ; Operand defined as vsrc_32
-    //              ; RI.getCommonSubClass(s0,vsrc_32) = sgpr ; LEGAL
-    //
-    // s_sendmsg 0, s0 ; Operand defined as m0reg
-    //                 ; RI.getCommonSubClass(s0,m0reg) = m0reg ; NOT LEGAL
-
-    return RI.getCommonSubClass(RC, RI.getRegClass(OpInfo.RegClass)) == RC;
+    return isLegalRegOperand(MRI, OpInfo, *MO);
   }
 
 
   // Handle non-register types that are treated like immediates.
   assert(MO->isImm() || MO->isTargetIndex() || MO->isFI());
 
   if (!DefinedRC) {
     // This operand expects an immediate.

 }
 
 void SIInstrInfo::legalizeOperands(MachineInstr *MI) const {
   MachineRegisterInfo &MRI = MI->getParent()->getParent()->getRegInfo();
   unsigned Opc = MI->getOpcode();
 
   // Legalize VOP2
 
-  if (isVOP2(MI->getOpcode())) {
-    int Src0Idx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::src0);
-    int Src1Idx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::src1);
-
-    // Legalize src0
-    if (!isOperandLegal(MI, Src0Idx))
-      legalizeOpWithMove(MI, Src0Idx);
+  const MCInstrDesc &InstrDesc = get(Opc);
 
-    // Legalize src1
-    if (isOperandLegal(MI, Src1Idx))
-      return;
-
-    // Usually src0 of VOP2 instructions allow more types of inputs
-    // than src1, so try to commute the instruction to decrease our
-    // chances of having to insert a MOV instruction to legalize src1.
+  if (isVOP2(Opc)) {
     if (MI->isCommutable()) {
-      if (commuteInstruction(MI))
+      if (commuteInstruction(MI)) {
         // If we are successful in commuting, then we know MI is legal, so
         // we are done.
         return;
       }
+    }
+
+    // VOP2 src0 instructions support all operand types, so we don't need to
+    // check their legality.
+    int Src1Idx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::src1);
+    MachineOperand &Src1 = MI->getOperand(Src1Idx);
 
+    if (!isLegalVSrcOperand(MRI, InstrDesc.OpInfo[Src1Idx], Src1))
       legalizeOpWithMove(MI, Src1Idx);
+
     return;
   }
 
   // Legalize VOP3
   if (isVOP3(Opc)) {
     legalizeOperandsVOP3(MRI, MI);
     return;
   }