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

Tue Apr 26 01:46:27 PDT 2022

rickyz marked an inline comment as done.
rickyz added a comment.

Apologies for my long-delayed response!

It seems that I was confused in my previous comments about the difficulty of exercising the non-strict equality cases - swapping the select arguments works as expected. D119689 <https://reviews.llvm.org/D119689> should now contain negative tests for each possible value of `Pred0`. The two non-strict equality tests pass even without this change, since the bug is only triggered when `Pred0 = ULT`.

================
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);
----------------
spatel wrote:
> 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.
Good idea, done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119690