[PATCH] D148957: [AMDGPU] Add intrinsics for converting global pointers to resources

[Krzysztof Drewniak via Phabricator via llvm-commits]

krzysz00 added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsAMDGPU.td:1009-1010
+
+def int_amdgcn_flat_as_buffer_rsrc : AMDGPUPtrAsBufferRsrc<0>;
+def int_amdgcn_global_as_buffer_rsrc : AMDGPUPtrAsBufferRsrc<1>;
+
----------------
arsenm wrote:
> There's no reason to have different intrinsics for different source address spaces. Just accept a type mangling operand for the input pointer 
1. Does "any pointer" work for different address spaces? The documentation's a bit fuzzy
2. If we're accepting arbitrary pointers, will we then need to, during legalization, reject pointer types that don't make sense (ex. LDS)?


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:5217-5218
+  // an abundance of caution.
+  case Intrinsic::amdgcn_flat_as_buffer_rsrc:
+  case Intrinsic::amdgcn_global_as_buffer_rsrc:
     return true;
----------------
arsenm wrote:
> This handling needs a test (I'm assuming that was the intent of ptr-buffer-alias-scheduling.ll, but I think we also need a pure IR one that doesn't depend on codegen)
The test over in LICM is handling this, though there might be a more straightforward way to do it.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4395
+bool AMDGPULegalizerInfo::legalizePointerAsRsrcIntrin(
+    MachineInstr &MI, MachineRegisterInfo &MRI, MachineIRBuilder &B) const {
+  Register Result = MI.getOperand(0).getReg();
----------------
gandhi21299 wrote:
> Might be helpful to have an assertion on the number of operands of `MI`. Is it possible that any of the operands is not a register?
>From what I can tell of all the surrounding code ... no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148957