[PATCH] D35107: Re-enable "[IndVars] Canonicalize comparisons between non-negative values and indvars"

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:268
     ICmp->setOperand(1, NewRHS);
+  } else if (ICmpInst::isSigned(OriginalPred) &&
+             SE->isKnownNonNegative(S) && SE->isKnownNonNegative(X)) {
----------------
mkazantsev wrote:
> anna wrote:
> > Could you please add a comment here why you're using the OriginalPred.
> > It seems useful to state it. 
> > 
> > Also,IIUC, doing this transform when Pred is swapped version of OriginalPred is error prone. We also need to check that IV is the second operand to avoid breaking canonicalization -- IV is always on the left. 
> That's what actually what the bug was. I used to use Pred here, which was wrong in the situation:
> 
>   sgt 100, iv
> 
> We wanted to operate with "slt iv, 100" within the method, but the tricky thing is that the instruction remained unchanged, we only replace the Pred variable and not operate in terms of left and right operand, but use this thing:
> 
>   const SCEV *S = SE->getSCEV(ICmp->getOperand(IVOperIdx));
>   const SCEV *X = SE->getSCEV(ICmp->getOperand(1 - IVOperIdx));
> 
> So in the described situation the actual instruction is  "sgt 100, iv", but the Pred variable is "slt". What we want to have in the end is "ugt 100, iv", that's why we need the original pred (or alternatively swap Pred twice in this case).
> 
> I will add a comment on that.
Thanks. 
Actually, what I meant was even enabling this transform *correctly* using `Pred` is error prone (but won't be a bug). The reason is precisely what you mentioned... we swap the predicate but not the operands. So, we need to swap the operands back, but only if Pred is swapped version of OriginalPred  *and* IV is on the right. As we can see depending on `Pred` is highly prone to errors :)
However, what you've done here is strictly correct, since we don't depend on knowing if `Pred` was changed in someway. 


https://reviews.llvm.org/D35107