[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!

7126–7128

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

7135–7139

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

[vpykhtin]

Per review fixes.

[vpykhtin]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7126–7128

I added the check before the loop.

[rampitec]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7136

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.

7149

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

7136

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

7147

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

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

[vpykhtin]

Fixed formattind and lint issues.

[rampitec]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7148

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

7108–7109

Why change to the uses instead of instructions?

[vpykhtin]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

7108–7109

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

7108–7109

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

 //===- SIInstrInfo.cpp - SI Instruction Information  ----------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 /// \file
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 // 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))

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;
+  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));
+}