[PATCH] D119690: [InstCombine] Update predicate when canonicalizing comparisons in canonicalizeClampLike.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a reviewer: lebedev.ri.
spatel added a comment.

@lebedev.ri mentioned that there may be 4 different patterns to test, but I'm not seeing how that happens. Suggestions?
It seems like we should be able to swap the select operands to make Pred0 become "ule", but then what other conditions are needed to trigger the bug?



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1405
     // so we don't have any restrictions on C2, so we can just handle it.
+    Pred1 = ICmpInst::Predicate::ICMP_SLT;
     std::swap(ReplacementLow, ReplacementHigh);
----------------
rickyz wrote:
> spatel wrote:
> > Why set this if it is never used below here?
> This isn't used, but I did it to signal that from this point forward, the predicate has been canonicalized to SLT. I see this as defense in depth against future bugs like this one (kind of like how some C programmers like to set pointer variables to NULL after freeing them). I can see how this could mislead a reader to believe that the variable is used afterwards though.
> 
> I'll remove it, since it seems like there's some preference against this.
I'm not opposed to the change, but if you add it, then there should be an assert below. That way, it is clear why we did the update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119690