[PATCH] D107330: [GlobalISel] Combine shr(shl x, c1), c2 to G_SBFX/G_UBFX

[Jay Foad via Phabricator via llvm-commits]

foad added a reviewer: foad.
foad added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h:544
 
+  /// Match: shr (shl x, n), k -> sbfx/ubfx x, n, width
+  bool matchBitfieldExtractFromShr(
----------------
This comment suggests that "n" is the same value on the LHS and the RHS, which is not true.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4155
+  // Try to match a constant on the RHS of the right shift
+  if (!mi_match(RHS, MRI, m_OneNonDBGUse(m_ICst(ShrAmt))))
+    return false;
----------------
You don't want a "one use" check for a constant.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4161
+  // Try to match shl x, cst on the left-hand side
+  if (!mi_match(LHS, MRI, m_OneNonDBGUse(m_GShl(m_Reg(LHS), m_ICst(ShlAmt)))))
+    return false;
----------------
I think it would be better to combine this and the previous mi_match into one large match expression.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4163-4164
+    return false;
+  if (ShlAmt >= Size)
+    return false;
+
----------------
Since ShlAmt is signed don't you also need to check it's >= 0? (TBH I'm not sure how C++ compares int64_t vs unsigned.)

Anyway you might be able to simplify some of the bounds checking. I **think** all you need to check is: `0 <= ShlAmt <= ShrAmt < Size`. Then the checks on Width and Pos should be unnecessary by construction.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4169
+  const int64_t Pos = ShrAmt - ShlAmt;
+  const int64_t Width = UpperBound - Pos;
+
----------------
This is just `Size - ShrAmt`!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107330