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

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added a comment.

In D101423#2723737 <https://reviews.llvm.org/D101423#2723737>, @lebedev.ri wrote:

> I have a really 'are you nuts' proposal.
> How about we canonicalize all i1 and/or's into `select` form?
> Then that instsimplify code becomes dead, and we can simply port it here.
> And we'll weed out all the remaining patterns that still only handle the and/or forms/

This will reduce a lot of complexity between having both select and or/and i1, but one concern is that select isn't commutative. :(
I mean, `and i1 %a, %b == and i1 %b, %a`, but in case of select `select i1 %a, %b, false != select i1 %b, %a, false` due to poison.
So we'll have to carefully choose which form to use, which can be a hard problem.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2587
 }
 
+/// The @llvm.[us]mul.with.overflow intrinsic could have been folded from some
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101423