[PATCH] D100430: [AMDGPU][GlobalISel] Widen 1 and 2 byte scalar loads

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:495
       ResTy, APInt::getLowBitsSet(ResTy.getScalarSizeInBits(), ImmOp));
-  return buildAnd(ResTy, Op, Mask);
+  return buildAnd(Res, Op, Mask);
 }
----------------
Should split this into a separate patch


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1157
+    const unsigned MemSize = 8 * MMO->getSize();
+    // Scalar loads of size 8 or 16 bit with proper alignment may be widen to 32
+    // bit. Check to see if we need to widen the memory access, 8 or 16 bit
----------------
Grammar, widen->widened


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1171-1172
+    if (LoadSize == 32) {
+      // A scalar load was legally widen to 32 bit but the memory access has not
+      // been widen yet to the correct size yet so widen it here to 32 bit.
+      if (MI.getOpcode() == AMDGPU::G_SEXTLOAD) {
----------------
This comment is worded confusingly so it doesn't explain what is going on. We are widening the memory access, not the result register type


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1175
+        // Must extend the sign bit into higher bits for a G_SEXTLOAD
+        const LLT S32 = LLT::scalar(32);
+        auto WideLoad = B.buildLoadFromOffset(S32, PtrReg, *MMO, 0);
----------------
Should just hoist the S32 definition instead of repeating it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100430