[PATCH] D147547: [AMDGPU] Add buffer intrinsics that take resources as pointers

[Krzysztof Drewniak via Phabricator via llvm-commits]

krzysz00 added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:134
 
+static LLT getBufferRsrcScalarType(const LLT Ty) {
+  if (!Ty.isVector())
----------------
changeElementType() exists


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:498
+  B.buildIntToPtr(MO, ScalarReg);
+  MO.setReg(BitcastReg);
+
----------------
arsenm wrote:
> Missing observer notification?
Should I pass the Observer in or have it called at the call sites or?

(Also, is it OK to `changingInstruction()/changedInstruction()` recursively?)


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:1027
   getActionDefinitionsBuilder(G_PTR_ADD)
-    .legalIf(all(isPointer(0), sameSize(0, 1)))
-    .scalarize(0)
-    .scalarSameSizeAs(1, 0);
+      .unsupportedFor({BufferFatPtr, RsrcPtr})
+      .legalIf(all(isPointer(0), sameSize(0, 1)))
----------------
arsenm wrote:
> Move this to the end, legal cases should be first and ordered with the most common cases first
Having checked, the legal rule matches before the unsupported, and the matching is done in order, so this needs to come first in order to make sure buffer pointer PTR_ADD gets caught in legalization (as opposed to relying on the fact that we currently can't select it)


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2479-2481
+  // Legalize vectors of large pointers to vectors of large integers so
+  // we don't try to do pointer <-> integer bitcasts later in legalization.
+  if (EltTy.isPointer() && EltTy.getSizeInBits() > 64) {
----------------
arsenm wrote:
> This seems very special cased and I don't understand why you need specially handle vector extracts
Updated the comment, and I think we need to handle them both for generality and since @nhaehnle mentioned they could come up in Vulkan


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147547