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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4405
+  B.setInsertPt(B.getMBB(), ++B.getInsertPt());
+  auto Unmerge = B.buildUnmerge(S32, Pointer);
+  Register LowHalf = Unmerge.getReg(0);
----------------
I thought you had to do B.buildUnmerge({S32, S32}, Pointer)?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4413-4414
+  MachineInstrBuilder NewHighHalf = Masked;
+  std::optional<ValueAndVReg> StrideConst =
+      getIConstantVRegValWithLookThrough(Stride, MRI);
+  if (!StrideConst.has_value() || !StrideConst->Value.isZero()) {
----------------
Do you need really need the version that returns APInt and the register, or can you use the one that returns int64_t?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4415
+      getIConstantVRegValWithLookThrough(Stride, MRI);
+  if (!StrideConst.has_value() || !StrideConst->Value.isZero()) {
+    MachineInstrBuilder ShiftedStride;
----------------
Can do !StrideConst


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4417
+    MachineInstrBuilder ShiftedStride;
+    if (StrideConst.has_value()) {
+      APInt ShiftedStrideVal = StrideConst->Value.zext(32).shl(16);
----------------
StrideConst


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:4418
+    if (StrideConst.has_value()) {
+      APInt ShiftedStrideVal = StrideConst->Value.zext(32).shl(16);
+      ShiftedStride = B.buildConstant(S32, ShiftedStrideVal.getZExtValue());
----------------
can you just get out of APInt? 


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:8676
+  SDValue NewHighHalf = Masked;
+  if (!ConstStride || ConstStride != 0) {
+    SDValue ShiftedStride;
----------------
Should this be *ConstStride != 0?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148957