[PATCH] D144313: [AMDGPU] Improve the lowering of raw_buffer_load_{i8,i16} and struct_buffer_load_{i8,i16} intrinsics

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Looks good with a few suggested changes.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCombinerHelper.cpp:400-404
+  if (SubwordBufferLoad->getOpcode() == AMDGPU::G_AMDGPU_BUFFER_LOAD_UBYTE ||
+      SubwordBufferLoad->getOpcode() == AMDGPU::G_AMDGPU_BUFFER_LOAD_USHORT)
+    return true;
+
+  return false;
----------------



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCombinerHelper.cpp:411-412
+    MachineInstr &MI, MachineInstr *&SubwordBufferLoad) {
+  Register Op0Reg = MI.getOperand(1).getReg();
+  SubwordBufferLoad = (*(MRI.def_begin(Op0Reg))).getParent();
+  // Modify the opcode and the destination of buffer_load_{u8, u16}:
----------------
Don't need these two lines. SubwordBufferLoad is passed in as an argument now.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCombinerHelper.cpp:416
+  const GCNSubtarget &Subtarget = MI.getMF()->getSubtarget<GCNSubtarget>();
+  const SIInstrInfo *TII = Subtarget.getInstrInfo();
+  unsigned Opc =
----------------
I think all this new code should probably live in `AMDGPUPostLegalizerCombinerHelper` instead of `AMDGPUCombinerHelper`, and you should add `TII` as a member variable in `AMDGPUPostLegalizerCombinerHelper` following the example of `AMDGPURegBankCombinerHelper`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144313