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

[Valery Pykhtin via Phabricator via llvm-commits]

vpykhtin created this revision.
vpykhtin added reviewers: foad, rampitec, arsenm.
Herald added subscribers: llvm-commits, kerbowa, hiraditya, t-tye, tpr, dstuttard, yaxunl, nhaehnle, jvesely, kzhuravl.
Herald added a project: LLVM.
vpykhtin requested review of this revision.
Herald added a subscriber: wdng.

I was wrong in thinking that MRI.use_instructions return unique instructions and mislead Jay in his previous patch D64393 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89386

Files:
  llvm/lib/Target/AMDGPU/SIInstrInfo.cpp


Index: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
===================================================================
--- llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -7093,7 +7093,6 @@
 
   return false;
 }
-
 bool llvm::execMayBeModifiedBeforeAnyUse(const MachineRegisterInfo &MRI,
                                          Register VReg,
                                          const MachineInstr &DefMI) {
@@ -7102,16 +7101,17 @@
   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;
   }
 
@@ -7120,18 +7120,31 @@
 
   // 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))
-      return true;
+      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;
+    }
   }
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D89386.298121.patch
Type: text/x-patch
Size: 2007 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201014/cb973638/attachment.bin>