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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm marked 2 inline comments as done.
arsenm added inline comments.


================
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;
 
----------------
nhaehnle wrote:
> Why the type change? DMaskLanes doesn't need to be signed, does it?
The parts below are using int, and it was less effort to swsitxch everything to int (which I guess is what the current guidance is anyway)


================
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);
----------------
nhaehnle wrote:
> 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?
I'm ignoring atomics for now until I have selection working for the normal load/stores


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

https://reviews.llvm.org/D73666