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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev 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)) {
----------------
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.


https://reviews.llvm.org/D35107