[PATCH] D12745: [IndVars] Widen more comparisons for non-negative induction vars.

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

replied inline.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:849
@@ -848,1 +848,3 @@
 
+  // True if the narrow induction variable is never negative.
+  bool NeverNegative;
----------------
reames wrote:
> A brief bit about what we can use this fact for would be good in the comment.  
Added a comment, ptal.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1265
@@ -1246,1 +1264,3 @@
 
+  NeverNegative = SE->isKnownPredicate(ICmpInst::ICMP_SGE, AddRec,
+                                       SE->getConstant(AddRec->getType(), 0));
----------------
reames wrote:
> Why not just SE->isKnownNonNegative(AddRec)?
`isKnownNonNegative` does not look at control flow and so is weaker than `isKnownPredicate`.  I've added a test case (`test8`) that does not work with `isKnownNonNegative`.

================
Comment at: test/Transforms/IndVarSimplify/widen-loop-comp.ll:213
@@ +212,3 @@
+for.body:
+  %idxprom = zext i32 %i.0 to i64
+  %arrayidx = getelementptr inbounds i32, i32* %a, i64 %idxprom
----------------
reames wrote:
> It isn't obvious to me whether we chose a sext or a sext for the induction variable in this loop.  We could do either base on order of visitation.  Could you clarify this?
> 
> Also, can you add tests for the full 2 x 2 matrix of signed/unsigned widdening x signed/unsigned compare?
> It isn't obvious to me whether we chose a sext or a sext for the
> induction variable in this loop. We could do either base on order of
> visitation. Could you clarify this?

Thanks for pointing this out!

In `@test7` and `@test6` it is impossible to tell what `-indvars` did
(sign extend or zero extend) based solely on how the structure of
`%indvars.iv`.  This is because normally you'd look at the initial
value of induction variable to deduce if the induction variable was
sign or zero extended; but here we're out of luck since the initial
value is a positive constant (== 0) so its sign and zero extensions
are the same.

I've added two more tests `@test8` and `@test9` that have a
non-constant initial value of the induction variable being widened.
By looking at the initial value of the induction variable phi (whether
it is a sext or zext), we can deduce if the indvar was zero extended
or sign extended.

If we start canonicalizing sign extensions of positive induction
variables to zero extensions then then it would be impossible tell if
zero or signed widening happened on `@test8` and `@test9` also; but we
do not do that today.

> Also, can you add tests for the full 2 x 2 matrix of signed/unsigned
> widdening x signed/unsigned compare?

The rest of the file already has tests for the two (signed widening,
signed compare) and (unsigned widening, unsigned compare) cases.
`@test8` checks the (signed extension, unsigned compare) case, while
`@test9` checks the (unsigned extension, signed compare) case.

Also, given your comment, I think `@test6` and `@test7` do not add
much value to this change beyond just being generic correctness tests.
Do you think it makes sense to remove these?



http://reviews.llvm.org/D12745