[PATCH] D91690: [LoopFlatten] Widen IV, cont'd

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:563
+    if (FI.Widened)
+      if (Value *U = HasSZExtUser(V))
+        V = U;
----------------
dmgreen wrote:
> SjoerdMeijer wrote:
> > dmgreen wrote:
> > > I'm not sure I understand any more. Should we not be replacing it with trunc(OuterInductionPHI) ?
> > > 
> > > Can you add a test where it (the o*I+i value) is not zext or sext?
> > > I'm not sure I understand any more. Should we not be replacing it with trunc(OuterInductionPHI) ?
> > 
> > After widening we have e.g. this pattern:
> > 
> >   %indvar = phi i64 [ %indvar.next, %for.body3.us ], [ 0, %for.cond1.preheader.us ]
> >   %3 = trunc i64 %indvar to i32
> >   %add.us = add i32 %3, %mul.us
> >   %idxprom.us = zext i32 %add.us to i64
> > 
> > The linear IV user in this example is:
> > 
> >   %add.us = add i32 %3, %mul.us
> > 
> > We don't want to be replacing this `%add.us` value which is a i32 value, because it will indeed be replaced by OuterInductionPhi, which is i64 value after widening. This was the assertion is was talking about. 
> > After widening, the value that we should be replacing is zext user which is `%idxprom.us` in this case.
> > After widening, we have this IV -> Trunc->LinearIV ->Ext pattern, which is what we are matching here.
> > 
> > I will add a comment to clarify this.
> > 
> > > Can you add a test where it (the o*I+i value) is not zext or sext?
> > 
> > I think these cases are present in the original test test/Transforms/LoopFlatten/loop-flatten.ll.
> > 
> > 
> Hmm. But, don't we start with something that looks like:
>   for i32 outer = 0..n
>     for i32 inner = 0..m
>       use(outer * m + inner)
> We widen the IV's so they become:
>   for i64 outer = 0..zext(n)
>     for i64 inner = 0..zext(m)
>       use(trunc(outer) * m + trunc(inner))
> And we want to replace that with a single
>   for i64 outer = 0..zext(n)*zext(m)
>     use(trunc(outer))
> We have not proved that the original did not overflow, so if it does we need to use the original truncated i32 value, not the i64 version of it directly.
Do you mean overflow in the original

  outer * m + inner

Expression? Is that relevant?

Will check when I am back at my desk, but I think after widening we have:

  Use(outer)

Without the trunc.


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

https://reviews.llvm.org/D91690