[PATCH] D125324: AMDGPU/GISel: Factor out AMDGPURegisterBankInfo::buildReadFirstLane

[Matt Arsenault via Phabricator via llvm-commits]

arsenm accepted this revision.
arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:719
+
+    const TargetRegisterClass *Constrained =
+        constrainGenericRegister(SrcPart, AMDGPU::VGPR_32RegClass, MRI);
----------------
kosarev wrote:
> Putting this under `#ifndef NDEBUG` would prevent calling `constrainGenericRegister()` in no-asserts builds.
Why would you want to do that? The operands always need to be constrained


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1017
   MachineIRBuilder B(MI);
+  B.setDebugLoc(MI.getDebugLoc());
 
----------------
Doesn't this happen in the constructor? (I do have a patch I Need to pick up to stop reconstructing this in RegBankSelect)


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/image-waterfall-loop-O0.ll:52
-; CHECK-NEXT:    buffer_store_dword v9, off, s[0:3], s32 offset:36 ; 4-byte Folded Spill
-; CHECK-NEXT:    v_mov_b32_e32 v9, v1
-; CHECK-NEXT:    v_mov_b32_e32 v8, v0
----------------
This looks like an improvement, but you also seem to have ended up with more instructions inside the loop (I guess this is -O0 so it doesn't really matter either way)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125324