[AMDGPU] Fix access beyond the end of the basic block in execMayBeModifiedBeforeAnyUse.

[vpykhtin]

Summary

I was wrong in thinking that MRI.use_instructions return unique instructions and mislead Jay in his previous patch D64393.

First loop counted more instructions than it was in reality and the second loop went beyond the basic block with that counter.

Jay, as you suggested, I used your previous code that relied on MRI.use_operands to constrain the number of instructions to check among.
I just inlined modifiesRegister to reduce the number of passes over instruction operands and added assert on BB end boundary.

I'm trying to make a test for this but this is hard.

[foad]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7096

Please leave the blank line!

7122

Is it possible to enter this loop with NumUse == 0? If so, I would expect the assert on the next line to fail.

7131–7132

Use a range-based for over I->operands() ?

[vpykhtin]

Per review fixes.

[vpykhtin]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7122

I added the check before the loop.

[rampitec]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7132

It will miss an exec subreg clobbers. Do we even use regmasks except calls? Then if it is a call it cannot be folded anyway.

7143

The indention suggests this return belongs to the else while it belongs to if. Missing braces.

[rampitec]

Actually do we have a test with a call in between def and use?

[vpykhtin]

In D89386#2330465, @rampitec wrote:

Actually do we have a test with a call in between def and use?

Probably not, how a call looks like at this stage?

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7132

I took this code from modifiesRegister, need to check.

[rampitec]

In D89386#2331649, @vpykhtin wrote:

In D89386#2330465, @rampitec wrote:

Actually do we have a test with a call in between def and use?

Probably not, how a call looks like at this stage?

It should be s_setpc_b64 at this point and it is a terminator and branch. Although I am not sure it can be detected as an EXEC change.

[vpykhtin]

In D89386#2332694, @rampitec wrote:

In D89386#2331649, @vpykhtin wrote:

In D89386#2330465, @rampitec wrote:

Actually do we have a test with a call in between def and use?

Probably not, how a call looks like at this stage?

It should be s_setpc_b64 at this point and it is a terminator and branch. Although I am not sure it can be detected as an EXEC change.

DPP combiner is constrained to work on a def and uses in one BB, I assume if a call is a terminator then there should be no uses after it.

[vpykhtin]

Added test, more review issues fixed.

[rampitec]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7141

I do not completely agree with clang tidy here, but I suggest to write "else if" on a single line and adjust indention accordingly.

llvm/unittests/Target/AMDGPU/ExecMayBeModifiedBeforeAnyUse.cpp

38 ↗

(On Diff #299082)

tidy is right here, "auto *" is preferable.

[vpykhtin]

Fixed formattind and lint issues.

[rampitec]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7142

Indention is off here.

[vpykhtin]

Fixed formatting issue

[rampitec]

LGTM

[vpykhtin]

Jay, Matt do you have objections with this patch?

[arsenm]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7107–7108

Why change to the uses instead of instructions?

[vpykhtin]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7107–7108

This is the core problem. instructions aren't guaranteed to be unique on iteration depending on the order of uses in use-def list for a register. See the unittest below that shows how this can happen.

[foad]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7107–7108

To put it another way: counting uses here allows you to exit early from the loop below when you have seen all the uses. Counting instructions here (with use_nodbg_instructions) does not work reliably because you might count some instructions twice depending on the order they appear in the use list.

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

if (++NumInst > MaxInstScan)
       return true;
 
     if (I->modifiesRegister(AMDGPU::EXEC, TRI))
       return true;
   }
 
   return false;
 }
-
 bool llvm::execMayBeModifiedBeforeAnyUse(const MachineRegisterInfo &MRI,
                                          Register VReg,
                                          const MachineInstr &DefMI) {
   assert(MRI.isSSA() && "Must be run on SSA");
 
   auto *TRI = MRI.getTargetRegisterInfo();
   auto *DefBB = DefMI.getParent();
 
-  const int MaxUseInstScan = 10;
-  int NumUseInst = 0;
+  const int MaxUseScan = 10;
+  int NumUse = 0;
 
-  for (auto &UseInst : MRI.use_nodbg_instructions(VReg)) {
+  for (auto &Use : MRI.use_nodbg_operands(VReg)) {
+    auto &UseInst = *Use.getParent();
     // Don't bother searching between blocks, although it is possible this block
     // doesn't modify exec.
     if (UseInst.getParent() != DefBB)
       return true;
 
-    if (++NumUseInst > MaxUseInstScan)
+    if (++NumUse > MaxUseScan)
       return true;
   }
 
   const int MaxInstScan = 20;
   int NumInst = 0;
 
   // Stop scan when we have seen all the uses.
   for (auto I = std::next(DefMI.getIterator()); ; ++I) {
+    assert(I != DefBB->end());
+
     if (I->isDebugInstr())
       continue;
 
     if (++NumInst > MaxInstScan)
       return true;
 
-    if (I->readsRegister(VReg))
-      if (--NumUseInst == 0)
-        return false;
+    for (unsigned OpI = 0, E = I->getNumOperands(); OpI != E; ++OpI) {
+      const auto &Op = I->getOperand(OpI);
 
-    if (I->modifiesRegister(AMDGPU::EXEC, TRI))
+      if (Op.isRegMask() && Op.clobbersPhysReg(AMDGPU::EXEC))
+        return true;
+
+      if (!Op.isReg())
+        continue;
+
+      Register Reg = Op.getReg();
+      if (Op.isUse()) {
+        if (Reg == VReg && --NumUse == 0)
+          return false;
+      } else // Reg is a def
+          if (TRI->regsOverlap(Reg, AMDGPU::EXEC))
         return true;
     }
   }
+}
 
 MachineInstr *SIInstrInfo::createPHIDestinationCopy(
     MachineBasicBlock &MBB, MachineBasicBlock::iterator LastPHIIt,
     const DebugLoc &DL, Register Src, Register Dst) const {
   auto Cur = MBB.begin();
   if (Cur != MBB.end())
     do {
       if (!Cur->isPHI() && Cur->readsRegister(Dst))