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:
> nikic wrote:
> > spatel wrote:
> > > 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?
> > I think this is one of the rare cases where inserting Freeze in InstCombine
> > makes sense. There's not much (any?) further optimization opportunities for
> > a umul.with.overflow with non-constant operands.
> InstCombine sounds good as well!
For reference, that part is D101423, and it will be a preliminary patch before
this one.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101191/new/
https://reviews.llvm.org/D101191
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
