[PATCH] D60772: [AMDGPU] Add optional bounds checking for scratch accesses

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

I don't understand the problem being solved here. Who/what is this intended to benefit? An out of bounds access is going to be undefined. I don't want to start trying to define in at an arbitrary point in the backend. Crashing on the invalid access is much easier problem to debug. This seems to be a partial replacement for asan? If this is intended as some user visible semantic fix, that requires more thought and probably needs to be a new address space.



================
Comment at: lib/Target/AMDGPU/SIInsertScratchBounds.cpp:210-228
+    if (ST->hasAddNoCarry()) {
+      BuildMI(*PreAccessBB, PreI, DL, TII->get(AMDGPU::V_ADD_U32_e32), AddrReg)
+        .addImm(Offset->getImm())
+        .addReg(Addr->getReg());
+    } else {
+      const unsigned OffsetReg =
+        MRI->createVirtualRegister(&AMDGPU::SReg_32RegClass);
----------------
You can use TII::getAddNoCarry


================
Comment at: lib/Target/AMDGPU/SIInsertScratchBounds.cpp:243-244
+      .addReg(SizeReg);
+    BuildMI(*PreAccessBB, PreI, DL,
+            TII->get(AMDGPU::S_AND_SAVEEXEC_B64), ExecReg)
+      .addReg(CondReg, getKillRegState(!IsLoad));
----------------
I'm working on only allowing instructions that are terminators to modify exec, but this is introducing a new exec write in the middle of a block


================
Comment at: lib/Target/AMDGPU/SIInsertScratchBounds.cpp:328-329
+        for (const auto &MMO : MI.memoperands()) {
+          const unsigned AddrSpace = MMO->getPointerInfo().getAddrSpace();
+          if (AddrSpace == AMDGPUAS::PRIVATE_ADDRESS) {
+            // uses scratch; needs to be processed
----------------
This isn't going to catch scratch accesses without an MMO, you need to check the opcodes


================
Comment at: lib/Target/AMDGPU/SIInsertScratchBounds.cpp:331-332
+            // uses scratch; needs to be processed
+            Worklist.push_back(&MI);
+            break;
+          }
----------------
I dislike building a worklist of all instructions in the function to handle. This shouldn't be hard to handle as you go through the function


================
Comment at: lib/Target/AMDGPU/SIInsertScratchBounds.cpp:341-342
+    const MachineFrameInfo &FrameInfo = MF.getFrameInfo();
+    const int64_t ScratchSizeEstimate =
+      (int64_t) FrameInfo.estimateStackSize(MF);
+
----------------
This isn't going to be strong enough to guarantee that there will never be an out of bounds access.

This approach also isn't going to work in a callable function, in the presence of calls, or variable sized stack objects (which I'm planning on implementing)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60772/new/

https://reviews.llvm.org/D60772