[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

7146

Please leave the blank line!

7176–7178

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

7185–7189

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

[vpykhtin]

Per review fixes.

[vpykhtin]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7176–7178

I added the check before the loop.

[rampitec]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7186

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.

7199

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

7186

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

7197

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

39

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

[vpykhtin]

Fixed formattind and lint issues.

[rampitec]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7198

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

7158–7159

Why change to the uses instead of instructions?

[vpykhtin]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7158–7159

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

7158–7159

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;
   }
 
+  if (NumUse == 0)
+    return false;
+
   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 (const MachineOperand &Op : I->operands()) {
+      // We don't check reg masks here as they're used only on calls:
+      // 1. EXEC is only considered const within one BB
+      // 2. Call should be a terminator instruction if present in a BB
 
-    if (I->modifiesRegister(AMDGPU::EXEC, TRI))
+      if (!Op.isReg())
+        continue;
+
+      Register Reg = Op.getReg();
+      if (Op.isUse()) {
+        if (Reg == VReg && --NumUse == 0)
+          return false;
+      } else 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))

llvm/unittests/Target/AMDGPU/CMakeLists.txt

 include_directories(
   ${PROJECT_SOURCE_DIR}/lib/Target/AMDGPU
   ${PROJECT_BINARY_DIR}/lib/Target/AMDGPU
   )
 
 set(LLVM_LINK_COMPONENTS
   AMDGPUCodeGen
   AMDGPUDesc
   AMDGPUInfo
   MC
   Support
   )
 
 add_llvm_target_unittest(AMDGPUTests
   DwarfRegMappings.cpp
+  ExecMayBeModifiedBeforeAnyUse.cpp
   )

llvm/unittests/Target/AMDGPU/ExecMayBeModifiedBeforeAnyUse.cpp

+//===- llvm/unittests/Target/AMDGPU/ExecMayBeModifiedBeforeAnyUse.cpp -----===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "AMDGPUSubtarget.h"
+#include "AMDGPUTargetMachine.h"
+#include "llvm/CodeGen/MachineModuleInfo.h"
+#include "llvm/CodeGen/TargetSubtargetInfo.h"
+#include "llvm/MC/MCTargetOptions.h"
+#include "llvm/Support/TargetRegistry.h"
+#include "llvm/Support/TargetSelect.h"
+#include "llvm/Target/TargetMachine.h"
+#include "gtest/gtest.h"
+#include <thread>
+
+using namespace llvm;
+
+// implementation is in the llvm/unittests/Target/AMDGPU/DwarfRegMappings.cpp
+std::unique_ptr<const GCNTargetMachine>
+createTargetMachine(std::string TStr, StringRef CPU, StringRef FS);
+
+TEST(AMDGPUExecMayBeModifiedBeforeAnyUse, TheTest) {
+  auto TM = createTargetMachine("amdgcn-amd-", "gfx906", "");
+  if (!TM)
+    return;
+
+  GCNSubtarget ST(TM->getTargetTriple(), std::string(TM->getTargetCPU()),
+                  std::string(TM->getTargetFeatureString()), *TM);
+
+  LLVMContext Ctx;
+  Module Mod("Module", Ctx);
+  Mod.setDataLayout(TM->createDataLayout());
+
+  auto *Type = FunctionType::get(Type::getVoidTy(Ctx), false);
+  auto *F = Function::Create(Type, GlobalValue::ExternalLinkage, "Test", &Mod);
+
+  MachineModuleInfo MMI(TM.get());
+  auto MF = std::make_unique<MachineFunction>(*F, *TM, ST, 42, MMI);
+  auto *BB = MF->CreateMachineBasicBlock();
+  MF->push_back(BB);
+
+  auto E = BB->end();
+  DebugLoc DL;
+  const auto &TII = *ST.getInstrInfo();
+  auto &MRI = MF->getRegInfo();
+
+  // create machine IR
+  Register R = MRI.createVirtualRegister(&AMDGPU::SReg_32RegClass);
+
+  MachineInstr *DefMI =
+      BuildMI(*BB, E, DL, TII.get(AMDGPU::S_MOV_B32), R).addImm(42).getInstr();
+
+  auto First =
+      BuildMI(*BB, E, DL, TII.get(AMDGPU::S_NOP)).addReg(R, RegState::Implicit);
+
+  BuildMI(*BB, E, DL, TII.get(AMDGPU::S_NOP)).addReg(R, RegState::Implicit);
+
+  // this violates the continuous sequence of R's uses for the first S_NOP
+  First.addReg(R, RegState::Implicit);
+
+#ifdef DEBUG_THIS_TEST
+  MF->dump();
+  MRI.dumpUses(R);
+#endif
+
+  // make sure execMayBeModifiedBeforeAnyUse doesn't crash
+  ASSERT_FALSE(execMayBeModifiedBeforeAnyUse(MRI, R, *DefMI));
+}