[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

[Sanjay Patel via Phabricator via cfe-commits]

spatel added inline comments.


================
Comment at: llvm/test/Transforms/PhaseOrdering/unsigned-multiply-overflow-check.ll:20
 
+; FIXME: noundef should be attached to args
 define i1 @will_not_overflow(i64 %arg, i64 %arg1) {
----------------
aqjune wrote:
> aqjune wrote:
> > spatel wrote:
> > > Any ideas about what it will take to get those argument attributes for the C++ source example shown in the code comment?
> > > 
> > > SDAG is still going to convert the `select` to `and`, so we can probably avoid regressions by replicating InstSimplify's omitCheckForZeroBeforeMulWithOverflow() as a DAG combine. Let me know if I should do that.
> > I promised to do the patch at D82317, but these days I'm occupied with other things, so it might not be a recent future (not in a month at least)...
> > 
> > I think it is a good chance to use freeze here: We can add
> > ```
> > %cond = icmp ne %x, 0
> > %v = call @llvm.umul.with.overflow(%x, %y)
> > %ov = extractvalue %v, 1
> > %res = select i1 %cond, %ov, false
> >   =>
> > %y.fr = freeze %y
> > %v = call @llvm.umul.with.overflow(%x, %y.fr)
> > %ov = extractvalue %v, 1
> > %res = %ov
> > ```
> > into CodeGenPrepare.
> > What do you think? CodeGenPrepare is already using freeze for a few transformations.
> Alive2 proof: https://alive2.llvm.org/ce/z/zgPUGT
I don't think we want to add code to CGP if we can avoid it. CGP is supposed to be a temporary hack that is not needed with GlobalISel. I do acknowledge that "temporary" in the code may outlive the people working on it today (!).

If we don't care about undef correctness in codegen (in other words, the select->and transform will live on in codegen forever), then we might as well add a DAG combine. Alternatively, are we comfortable creating freeze in instcombine for rare code like umul.with.ov?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101191