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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm requested changes to this revision.
arsenm added a comment.
This revision now requires changes to proceed.

Needs some pure MIR tests too (also including negative tests)



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:448
+  // Require 4-byte alignment.
+  return MMO->getSize() >= 1 && MMO->getAlign() >= Align(4) &&
          // Can't do a scalar atomic load.
----------------
There's no point in checking the size anymore


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1159
+    // bit.
+    if (LoadSize == 32 && (MMO->getSize() >= 4 ||
+			    MI.getOpcode() != AMDGPU::G_LOAD ||
----------------
I don't understand this getSize check. It's illegal for the memory size to exceed the loaded type


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1160
+    if (LoadSize == 32 && (MMO->getSize() >= 4 ||
+			    MI.getOpcode() != AMDGPU::G_LOAD ||
+			    LoadTy.isVector() ||
----------------
You could also handle G_SEXTLOAD and G_ZEXTLOAD, but would require inserting some instructions to set the high bits appropriately. This also makes me think this should be done earlier


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1171
+    if (LoadSize == 32) {
+      // Widen memory access for 8 or 16 bit scalar loads to 32 bit.
+      LLT WiderTy = LLT::scalar(32);
----------------
Needs to elaborate more on when this is valid


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1174
       auto WideLoad = B.buildLoadFromOffset(WiderTy, PtrReg, *MMO, 0);
       B.buildExtract(MI.getOperand(0), WideLoad, 0);
+    } else {
----------------
Should use trunc. I'm trying to eliminate G_EXTRACT usage


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100430