[PATCH] D87050: GlobalISel: Add combines for G_TRUNC

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1845
+    return true;
+  } else {
+    Builder.setInstrAndDebugLoc(MI);
----------------
No else after return


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1867
+  LLT ShiftAmtTy = getTargetLowering().getPreferredShiftAmountTy(DstTy);
+  if (MRI.hasOneUse(SrcReg) &&
+      mi_match(SrcReg, MRI, m_GShl(m_Reg(ShiftSrc), m_Reg(ShiftAmt))) &&
----------------
hasOneNonDBGUse?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1869
+      mi_match(SrcReg, MRI, m_GShl(m_Reg(ShiftSrc), m_Reg(ShiftAmt))) &&
+      isLegalOrBeforeLegalizer({TargetOpcode::G_SHL, {DstTy, ShiftAmtTy}})) {
+    KnownBits Known = KB->getKnownBits(ShiftAmt);
----------------
For AMDGPU this would need a regbank legality check for post-regbankselect combines but I'm not sure what to do about that


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1891
+  Builder.setInstrAndDebugLoc(MI);
+  Builder.buildInstr(TargetOpcode::G_SHL, {DstReg},
+                     {Builder.buildTrunc(DstTy, ShiftSrc),
----------------
buildShl


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/shl.ll:85-86
 ; GFX8:       ; %bb.0:
-; GFX8-NEXT:    s_and_b32 s0, s0, 0xff
-; GFX8-NEXT:    s_lshl_b32 s0, s0, 7
+; GFX8-NEXT:    s_bfe_u32 s1, 7, 0x100000
+; GFX8-NEXT:    s_lshl_b32 s0, s0, s1
 ; GFX8-NEXT:    ; return to shader part epilog
----------------
This is strange looking. It's technically neutral but the shift amount no longer appears constant?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87050