[PATCH] D64285: [InstCombine] Fold select (icmp sgt x, -1), lshr (X, Y), ashr (X, Y) to ashr (X, Y))

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

It'd be really great to have at just least one positive non-splat-vector test for instsimplify and instcombine for each predicate (i.e. 4 more tests?)



================
Comment at: lib/Analysis/InstructionSimplify.cpp:92-93
+        match(CmpLHS, m_Specific(X))) {
+      if (!cast<BinaryOperator>(TrueVal)->isExact() &&
+          cast<BinaryOperator>(FalseVal)->isExact())
+        return nullptr;
----------------
xbolva00 wrote:
> xbolva00 wrote:
> > lebedev.ri wrote:
> > > lebedev.ri wrote:
> > > > ```
> > > > /// If either the `ashr` is non-`exact`, or `lshr` is exact too, then we can just return the `ashr` instruction.
> > > > ```
> > > > i think it may be less confusing to flip the `if` accordingly.
> > > Hmm, also, as i've recently seen, `cast<BinaryOperator>` is going to break for constant expressions.
> > > Let's just do the most obvious thing instead:
> > > ```
> > > if (match(TrueVal, m_LShr(m_Value(X), m_Value(Y))) &&
> > >     match(FalseVal, m_AShr(m_Specific(X), m_Specific(Y))) &&
> > >     match(CmpLHS, m_Specific(X)) && 
> > >     (!match(FalseVal, m_Exact()) || match(TrueVal, m_Exact())))
> > > ```
> > m_Exact() not exists?
> m_Exact(m_BinOp()).. okay
Ugh, right, sorry.
Though maybe `m_Instruction()`, not `m_BinOp()`; this really asks for `m_Anything()` matcher..


================
Comment at: lib/Analysis/InstructionSimplify.cpp:82-84
+  if ((Pred != ICmpInst::ICMP_SGT || !match(CmpRHS, m_APInt(C)) ||
+       !C->sge(-1)) &&
+      (Pred != ICmpInst::ICMP_SLT || !match(CmpRHS, m_APInt(C)) || !C->sge(0)))
----------------
This is not going to work with non-splat vector constants, although it is trivial to make it,
i'd recommend to do this instead:
```
  unsigned Bitwidth = CmpRHS->getType()->getScalarSizeInBits();
  if ((Pred != ICmpInst::ICMP_SGT ||
       !match(CmpRHS,
              m_SpecificInt_ICMP(ICmpInst::ICMP_SGE, APInt(Bitwidth, -1)))) &&
      (Pred != ICmpInst::ICMP_SLT ||
       !match(CmpRHS,
              m_SpecificInt_ICMP(ICmpInst::ICMP_SGE, APInt(Bitwidth, 0))))) {
```
(`m_SpecificInt_ICMP()` is really fresh)


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:548-553
+  const APInt *C;
+  if ((Pred != ICmpInst::ICMP_SGT || !match(CmpRHS, m_APInt(C)) ||
+       !C->sge(-1)) &&
+      (Pred != ICmpInst::ICMP_SLT || !match(CmpRHS, m_APInt(C)) || !C->sge(0)))
+    return nullptr;
+
----------------
Same remark about constant


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:561
+      match(CmpLHS, m_Specific(X))) {
+    const auto *Ashr = cast<BinaryOperator>(FalseVal);
+    // This new ashr must not be exact.
----------------
Likewise, i think it's sufficient to cast to `Instruction`..


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:564
+    return Builder.CreateAShr(Ashr->getOperand(0), Ashr->getOperand(1),
+                              Ashr->getName(), /*isExact=*/false);
+  }
----------------
nice!


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

https://reviews.llvm.org/D64285