[PATCH] D101423: [InstCombine] Fold overflow bit of [u|s]mul.with.overflow in a poison-safe way

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2587
 }
 
+/// The @llvm.[us]mul.with.overflow intrinsic could have been folded from some
----------------
aqjune wrote:
> spatel wrote:
> > aqjune wrote:
> > > spatel wrote:
> > > > aqjune wrote:
> > > > > The function below is based on InstSimplify's version.
> > > > Duplicating so much code feels wrong. Could we just call the simplify code from here with something like:
> > > > 
> > > > ```
> > > >     // Simulate a bitwise logic op for a select with a constant operand. 
> > > >     if (match(FalseVal, m_Zero())) {
> > > >       if (Value *Simplified =
> > > >               SimplifyAndInst(CondVal, TrueVal, SQ.getWithInstruction(&SI))) {
> > > >         // create freeze and return
> > > >       }
> > > >     }
> > > >     if (match(TrueVal, m_One())) {
> > > >       if (Value *Simplified =
> > > >               SimplifyOrInst(CondVal, FalseVal, SQ.getWithInstruction(&SI))) {
> > > >         // create freeze and return
> > > >       }
> > > >     }
> > > > 
> > > > ```
> > > I think we still need to look into the expression because we should determine which variable to freeze.
> > > 
> > > If there is a header file that InstSimplify and InstCombine can share would be a great place for this function?
> > Don't we only need to freeze the operand that is not in the icmp expression? That is, CondVal of the select must match as an icmp with a non-zero operand, and it's the other value that must be frozen.
> > 
> > I don't know of a shared header in Analysis for general logic. We do have things like VectorUtils and CmpInstAnalysis, so we could invent some new place.
> > Don't we only need to freeze the operand that is not in the icmp expression? That is, CondVal of the select must match as an icmp with a non-zero operand, and it's the other value that must be frozen.
> 
> Yep, the variable is the one to freeze.
> My concern was about code duplication: the pattern matching code should still be copied here to pick the variable...
> 
> > I don't know of a shared header in Analysis for general logic. We do have things like VectorUtils and CmpInstAnalysis, so we could invent some new place.
> 
> Actually to me having a separate header like CmpInstAnalysis and VectorUtil looks great.
> Would creating something like OverflowInstAnalysis make sense?
> If it is too much, having InstSimplifyUtils.h in llvm/lib/Analysis & including it from InstCombineSelect.cpp?
An Overflow analysis file sounds good. At this point, I think there's only the `mul` code that would go in there, but we might find other common code later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101423