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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:482
+    Register VectorReg = MRI.createGenericVirtualRegister(VectorTy);
+    SmallVector<Register, 4> VectorElems;
+    B.setInsertPt(B.getMBB(), ++B.getInsertPt());
----------------
Can just use an std::array since if this is only 4 piece case?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:484-486
+    for (unsigned I = 0; I < NumParts; ++I) {
+      VectorElems.push_back(MRI.createGenericVirtualRegister(S32));
+      B.buildExtractVectorElementConstant(VectorElems.back(), VectorReg, I);
----------------
Can do VectorElements[I]= B.buildExtractVectorElementConstant(S32, ...).getReg().

Also we really should have a scalarize vector utility in MachineIRBuilder like the DAG does


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:496
+  B.setInsertPt(B.getMBB(), ++B.getInsertPt());
+  B.buildBitcast(ScalarReg, BitcastReg);
+  B.buildIntToPtr(MO, ScalarReg);
----------------
auto BitCast = B.buildBitcast(ScalarTy, BitcastReg)


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:498
+  B.buildIntToPtr(MO, ScalarReg);
+  MO.setReg(BitcastReg);
+
----------------
Missing observer notification?


================
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)))
----------------
Move this to the end, legal cases should be first and ordered with the most common cases first


================
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) {
----------------
This seems very special cased and I don't understand why you need specially handle vector extracts


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2487
+    Register IntVec = MRI.createGenericVirtualRegister(IntVecTy);
+    B.buildPtrToInt(IntVec, Vec);
+
----------------
Fold register creation into the build 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2525-2526
 
+  Register Dst = MI.getOperand(0).getReg();
+  Register Vec = MI.getOperand(1).getReg();
+  Register Ins = MI.getOperand(2).getReg();
----------------
Ditto with the extract case


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/fp64-atomics-gfx90a.ll:3
 ; RUN: llc -global-isel -march=amdgcn -mcpu=gfx90a -verify-machineinstrs < %s | FileCheck %s -check-prefix=GFX90A
+; RUN: llc < %s -march=amdgcn -mcpu=gfx940 -verify-machineinstrs | FileCheck %s -check-prefix=GFX940
 
----------------
Add new run line in pre-commit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147547