[PATCH] D73666: AMDGPU/GlobalISel: Adjust image load register type based on dmask

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

Mostly looks good, but I do have some questions.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:3201
   const bool IsD16 = Ty.getScalarType() == S16;
-  const unsigned NumElts = Ty.isVector() ? Ty.getNumElements() : 1;
+  const int NumElts = Ty.isVector() ? Ty.getNumElements() : 1;
 
----------------
Why the type change? DMaskLanes doesn't need to be signed, does it?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:3210
 
-    // The raw dword aligned data component of the load. The only legal cases
-    // where this matters should be when using the packed D16 format, for
-    // s16 -> <2 x s16>, and <3 x s16> -> <4 x s16>,
-    LLT RoundedTy;
-    LLT TFETy;
+  const unsigned AdjustedNumElts = DMaskLanes == 0 ? 1 : DMaskLanes;
+  const LLT AdjustedTy = Ty.changeNumElements(AdjustedNumElts);
----------------
This seems like it might mess with atomics.

RMW atomics are 32- or 64-bits depending on whether dmask is 1 or 3. cmpswap atomics are 32- or 64-bits depending on whether dmask is 3 or 15.

I guess you're not testing atomics on the GlobalISel path yet?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:3254
 
-    SmallVector<Register, 5> UnmergeResults(TFETy.getNumElements(), Dst1Reg);
-    int NumDataElts = TFETy.getNumElements() - 1;
+  // FIXME: Do we need to notify the observer of the instruction change?
+  MI.getOperand(0).setReg(NewResultReg);
----------------
You do now have an Observer argument here, right?


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

https://reviews.llvm.org/D73666