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

[Carl Ritson via Phabricator via llvm-commits]

critson marked 6 inline comments as done.
critson added inline comments.


================
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));
----------------
arsenm wrote:
> 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
This code inserts new basic blocks such that only terminators modify exec, unless I missed something?


================
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
----------------
arsenm wrote:
> This isn't going to catch scratch accesses without an MMO, you need to check the opcodes
I don't think we can do anything sensible with instructions without MMO.  They could be accessing any address space, so it would not be appropriate to apply scratch bounds checks to them.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60772