[PATCH] D74141: [InstCombine] Simplify a umul overflow check to a != 0 && b != 0.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2726-2743
+  CmpInst::Predicate Pred;
+  Value *UMul1, *UMul2;
+  // Check if the OR weakens the overflow condition for umul.with.overflow by
+  // treating any non-zero result as overflow. In that case, we overflow if both
+  // umul.with.overflow operands are != 0, as in that case the result can only
+  // be 0, iff the multiplication overflows.
+  auto P = m_ICmp(Pred, m_ExtractValue<0>(m_Value(UMul2)), m_SpecificInt(0));
----------------
lebedev.ri wrote:
> fhahn wrote:
> > lebedev.ri wrote:
> > > fhahn wrote:
> > > > lebedev.ri wrote:
> > > > > lebedev.ri wrote:
> > > > > > Consider doing the following instead:
> > > > > > ```
> > > > > >   // Check if the OR weakens the overflow condition for umul.with.overflow by
> > > > > >   // treating any non-zero result as overflow. In that case, we overflow if both
> > > > > >   // umul.with.overflow operands are != 0, as in that case the result can only
> > > > > >   // be 0, iff the multiplication overflows.
> > > > > >   CmpInst::Predicate Pred;
> > > > > >   Value *MulWithOv;
> > > > > >   if (match(&I,
> > > > > >             m_c_Or(m_ICmp(Pred, m_OneUse(m_ExtractValue<0>(m_Value(MulWithOv))),
> > > > > >                           m_ZeroInt()),
> > > > > >                    m_OneUse(m_ExtractValue<1>(m_Deferred(MulWithOv))))) &&
> > > > > >       Pred == CmpInst::ICMP_NE) {
> > > > > >     Value *A, *B;
> > > > > >     if (match(MulWithOv, m_Intrinsic<Intrinsic::umul_with_overflow>(
> > > > > >                              m_Value(A), m_Value(B))))
> > > > > >       return BinaryOperator::CreateAnd(
> > > > > >           Builder.CreateICmpNE(A, ConstantInt::get(A->getType(), 0)),
> > > > > >           Builder.CreateICmpNE(A, ConstantInt::get(B->getType(), 0)));
> > > > > >   }
> > > > > > ```
> > > > > Also, use `Builder.CreateIsNotNull()`
> > > > Nice, m_c_Or and m_Deferred are very helpful!
> > > Please adjust one-use checks as they were in the snippet,
> > > both operands of `or` must be single-use, and we don't care if the intrinsic goes away or not.
> > > Please adjust one-use checks as they were in the snippet, both operands of or must be single-use
> > 
> > Do you mean both `extractvalues`? I am not sure, why do we need to  restrict the number of uses for the multiplication result? 
> > 
> > > and we don't care if the intrinsic goes away or not.
> > 
> > Will this still be profitable if the simplification does not result in the umul.with.overflow call to go away?
> Actually wait, i see where this is going.
> So, we need to produce 3 instructions (`&`, `!=0`, `!=0`),
> which means that we need to be sure that we eliminate two instructions.
> The input pattern is
> ```
>   %res = tail call { i64, i1 } @llvm.umul.with.overflow.i64(i64 %a, i64 %b)
>   %overflow = extractvalue { i64, i1 } %res, 1
>   %mul = extractvalue { i64, i1 } %res, 0
>   %cmp  = icmp ne i64 %mul, 0
>   %overflow.1 = or i1 %overflow, %cmp
> ```
> `%overflow.1` is free to replace, and we need two extras.
> They must be it's operands.
> * If the `%overflow` is one-use, then only the `%mul` result of `%res` is used,
>   which guarantees that `%mul`&`%res` will be combined into a simple `mul` instruction,
>   thus getting rid of the `%mul` instruction.
> * Even if `%mul` is one-use, `%overflow` might not be (if it is, see above), so we don't gain anything here.
> 
> TLDR: we only need a single use check, on `%overflow = extractvalue { i64, i1 } %res, 1`.
> `%mul = extractvalue { i64, i1 } %res, 0` is either also one-use and will go away,
> or it will get folded into `%res`, either way we end up not increasing instruction count.
Ah, i forgot about `%cmp`.
* if `%cmp` is one-use and `%mul` is also one-use we also can transform.
 (if `%mul` isn't one-use `%overflow` would need to be, but then see above)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74141