[PATCH] D158463: [AMDGPU] Add IR-level pass to rewrite away address space 7

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp:24
+// semantics. This allows the frontend to reason about such buffers (which are
+// often encounterd in the context of SPIR-V kernels).
+//
----------------



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp:37
+// encountering cases where this is required, such as by inserting these values
+// into aggretates ormoving them to memory.
+//
----------------



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp:678
+      NewArg = CE->getIntegerCast(NewArg, OffTy, /*IsSigned=*/true);
+      if (Multiple.isPowerOf2())
+        NewArg = ConstantExpr::getShl(
----------------
Braces 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp:684
+            /*hasNUW=*/InBounds, /*HasNSW=*/InBounds);
+      else
+        NewArg =
----------------
Braces


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp:688
+                                 /*hasNUW=*/InBounds, /*hasNSW=*/InBounds);
+      if (OffAccum)
+        OffAccum = ConstantExpr::getAdd(OffAccum, NewArg, /*hasNUW=*/InBounds,
----------------
Braces


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp:1009
+
+    std::optional<Value *> MaybeRsrc = std::nullopt;
+    auto MaybeFoundRsrc = FoundRsrcs.find(I);
----------------



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp:1018
+      getPossibleRsrcRoots(I, Roots, Seen);
+      LLVM_DEBUG(llvm::dbgs() << "Processing conditional: " << *I << "\n");
+#ifndef NDEBUG
----------------



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp:1021
+      for (Value *V : Roots)
+        LLVM_DEBUG(llvm::dbgs() << "Root: " << *V << "\n");
+      for (Value *V : Seen)
----------------



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp:1023
+      for (Value *V : Seen)
+        LLVM_DEBUG(llvm::dbgs() << "Seen: " << *V << "\n");
+#endif
----------------



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp:1135
+      } else {
+        delete OffDbg;
+      }
----------------
Don't think you're supposed to use a raw delete on any llvm value? Is there an erase method of some kind?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp:1175
+  if (IsVolatile)
+    Intr->setMetadata("amdgpu.volatile", MDNode::get(Ctx, {}));
+}
----------------
What is this for? The safer thing to do would be record not volatile in metadata 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerBufferFatPointers.cpp:298
+  }
+  bool IsUniqued = !isa<StructType>(Ty) || cast<StructType>(Ty)->isLiteral();
+  // Base case for ints, floats, opaque pointers, and so on, which don't
----------------
arsenm wrote:
> dyn_cast? Also could make a predicate function and clarify what uniqued means 
not done?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158463