[PATCH] D136319: [GISel] Rework trunc/shl combine in a generic trunc/shift combine

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h:415
+  ///    if K <= (MidVT.getScalarSizeInBits() - VT.getScalarSizeInBits())
+  /// MidVT is obtained from TLI using getPreferredShiftAmountTy, so the
+  /// trunc/right shift combine is only applied when that returns a type
----------------
foad wrote:
> Interesting - this seems like an abuse of getPreferredShiftAmountTy, but I guess it works out OK in practice for the one case we care about (converting 64-bit shifts to 32-bit shifts).
This is definitely not what getPreferredShiftAmountTy is for, which is only supposed to help produce already legal shift RHS operands


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:2283
+            {TargetOpcode::G_SHL,
+             {DstTy, getTargetLowering().getPreferredShiftAmountTy(DstTy)}}))
+      return false;
----------------
This is what getPreferredShiftAmountTy is for


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:2297
+    // Try and reduce the shift when it's profitable according to the target.
+    LLT NewShiftTy = getTargetLowering().getPreferredShiftAmountTy(SrcTy);
+    const unsigned NewShiftSize = NewShiftTy.getScalarSizeInBits();
----------------
This usage of getPreferredShiftAmountTy doesn't make sense, but you need something to pick an intermediate type here. Without adding anything, I guess you could bisect types until you found a legal shift? Creating the minimum required shift also "should" work out, but may create more legalizer and optimizer work. It would be nice if we could directly inspect the list of legal shift types, but the API doesn't have that.

Does just reusing the truncating type work as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136319