[PATCH] D60649: [InstCombine] Remove redundant/bogus mul_with_overflow folds

Sat Apr 13 04:20:15 PDT 2019

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

In D60649#1465394 <https://reviews.llvm.org/D60649#1465394>, @nikic wrote:

> In D60649#1465387 <https://reviews.llvm.org/D60649#1465387>, @lebedev.ri wrote:
>
> > In D60649#1465383 <https://reviews.llvm.org/D60649#1465383>, @nikic wrote:
> >
> > > @lebedev.ri Those aren't constant folds though. They fold to `{%x, false}`, which has to be created as something like `insertvalue {undef, false}, 0, %x`, which is non-constant. Or is the idea here more along the lines of folding `extractvalue addo(%x, 0), 1` to false in instsimplify? That would be possible, but doesn't seem like the right thing to do to me.
> >
> >
> > Uhm, i think i have used wrong word.
> >  What i meant is that those folds don't create new instructions (but simply return
> >  one of the arguments, in this case), therefore they should be in instsimplify.
> >  That's the distinction between instcombine vs instsimplify.
>
>
> These folds create new instructions: An insertvalue is needed to create the aggregate. These folds can only be performed in instsimplify is we start matching from extractvalue rather than with.overflow, as we don't need to explicitly construct the aggregate in that case. If we do so, the instcombine folds are still needed though, because we don't necessarily extract from the aggregate (could also return etc.)

Oh wait, i'm reading the https://github.com/llvm-mirror/llvm/blob/bd8056ef326e075cc500f3f0cfcd1193bc200594/lib/Analysis/InstructionSimplify.cpp#L4750-L4757 wrong.
Indeed, despite how they look, they don't already produce `insertvalue` https://godbolt.org/z/sb_xoT (i thought they did)
So yes, it wouldn't be as simple as just moving these folds from here to there.

Okay, lg.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60649