[AMDGPU] SIOptimizeExecMaskingPreRA should check constant bus constraint when folds EXEC copy

[alex-t]

Summary

Folding EXEC copy into it's single use may lead to constant bus constraint violation as it adds one more SGPR operand.

This change makes it validate the user instruction with the new SGPR operand and only fold it if it is legal.

[rampitec]

Needs test.

llvm/lib/Target/AMDGPU/SIOptimizeExecMaskingPreRA.cpp

421

This could be an implicit use by VALU, which shall be ignored then.

[alex-t]

Added MIR test. Implicit operand check added.

[alex-t]

With the change test checks for the COPY but w/o change, it reports "Bad machine code" error because of the "-verify-machineinstrs".
It may be changed just to report "expected string not found..." by removing "-verify-machineinstrs".

[arsenm]

llvm/test/CodeGen/AMDGPU/opt_exec_copy_fold.mir

16 ↗

(On Diff #331977)

Is exec + implicit exec + other SGPR really a constant bus violation?

[rampitec]

llvm/lib/Target/AMDGPU/SIOptimizeExecMaskingPreRA.cpp

424

It is OK for exec operand to be implicit, but you do not need to check for legality in this case.

[alex-t]

llvm/lib/Target/AMDGPU/SIOptimizeExecMaskingPreRA.cpp

424

The EXEC register may be an implicit operand but the "Idx" is the index of the Sreg64 register that holds the EXEC copy.

[rampitec]

LGTM

llvm/lib/Target/AMDGPU/SIOptimizeExecMaskingPreRA.cpp

424

Yep, you are right.

llvm/test/CodeGen/AMDGPU/opt_exec_copy_fold.mir

16 ↗

(On Diff #331977)

I think SGPR + EXEC is a violation, implicit exec is just an internal glue. There is difference if you read exec as exec or as data operand.

[alex-t]

Matt, do you still have any objections? Could you please unlock the review if no?

llvm/lib/Target/AMDGPU/SIOptimizeExecMaskingPreRA.cpp

for (MachineBasicBlock &MBB : MF) {
     unsigned ScanThreshold = 10;
     for (auto I = MBB.rbegin(), E = MBB.rend(); I != E
          && ScanThreshold--; ++I) {
       // Continue scanning if this is not a full exec copy
       if (!(I->isFullCopy() && I->getOperand(1).getReg() == Register(ExecReg)))
         continue;
 
       Register SavedExec = I->getOperand(0).getReg();
-      if (SavedExec.isVirtual() && MRI->hasOneNonDBGUse(SavedExec) &&
-          MRI->use_instr_nodbg_begin(SavedExec)->getParent() ==
-              I->getParent()) {
+      if (SavedExec.isVirtual() && MRI->hasOneNonDBGUse(SavedExec)) {
+        MachineInstr *SingleExecUser = &*MRI->use_instr_nodbg_begin(SavedExec);
+        int idx = SingleExecUser->findRegisterUseOperandIdx(SavedExec);
+        assert(idx != -1);
+        if (SingleExecUser->getParent() == I->getParent() &&
+            TII->isOperandLegal(*SingleExecUser, idx, &I->getOperand(1))) {
           LLVM_DEBUG(dbgs() << "Redundant EXEC COPY: " << *I << '\n');
           LIS->RemoveMachineInstrFromMaps(*I);
           I->eraseFromParent();
           MRI->replaceRegWith(SavedExec, ExecReg);
           LIS->removeInterval(SavedExec);
           Changed = true;
         }
+      }
       break;
     }
   }
 
   if (Changed) {
     for (auto Reg : RecalcRegs) {
       if (Reg.isVirtual()) {
         LIS->removeInterval(Reg);