[PATCH] D119715: [InstCombine] Fold sub(0,and(lshr(X,C),1)) --> ashr(shl(X,(BW-1)-C),BW-1) (PR53610)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Hm, please also add an assortment of tests where there is a `trunc` inbetween, we really should handle that.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp:192
+    // sub(C1,and(lshr(X,C2),1)) --> add(ashr(shl(X,(BW-1)-C2),BW-1),C1)
+    if (I->hasOneUse() && match(Ops[1], m_One()) &&
+        match(Ops[0], m_OneUse(m_LShr(m_Value(X), m_ImmConstant(ShAmt))))) {
----------------
Since one-use check is required, this should go into the next switch which is use-constrained.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp:194
+        match(Ops[0], m_OneUse(m_LShr(m_Value(X), m_ImmConstant(ShAmt))))) {
+      Constant *Ofs = ConstantInt::get(X->getType(), BitWidth - 1);
+      Value *NewShAmt = Builder.CreateSub(Ofs, ShAmt);
----------------
SplattingShAmt


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp:193
+    if (match(Ops[1], m_One()) &&
+        match(Ops[0], m_LShr(m_Value(X), m_Constant(ShAmt)))) {
+      Constant *Ofs = ConstantInt::get(X->getType(), BitWidth - 1);
----------------
spatel wrote:
> Use `m_ImmConstant` unless there's a test/need to match a constant expression.
I think just plain `m_c_And()` would make more sense here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119715