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

[Ricky Zhou via Phabricator via llvm-commits]

rickyz added inline comments.


================
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:
> 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.


================
Comment at: llvm/test/Transforms/InstCombine/canonicalize-clamp-like-pattern-between-negative-and-positive-thresholds.ll:216
   %t1 = select i1 %t0, i32 %replacement_low, i32 %replacement_high
   %t2 = icmp uge i32 %x, 128
   %r = select i1 %t2, i32 %x, i32 %t1
----------------
spatel wrote:
> I don't think this variation adds real coverage for the fix. 'uge' will be canonicalized to 'ugt' before we reach the buggy code, so it's the same as the previous test. 
> 
> Do we need to swap the select operands to exercise the predicate cases further? It's not clear to me from just looking at the code or the existing tests how to hit the bug.
> 
> 
Ah, I misinterpreted the comment as asking for tests of different predicates regardless of whether they hit the switch cases in this code (i.e. to provide coverage should canonicalization change at some point). Thinking about this more, I think I also lean towards deleting these tests.

I don't know of a good way to exercise the UGE and ULE cases in the switch on `Pred0` - like you mentioned, comparisons against a constant are canonicalized to strict comparisons in `canonicalizeCmpWithConstant`. The only cases I can think of where this canonicalization will not happen are tautological boundary cases like `icmp ule i32 %x, -1` and `icmp uge i32 %x, 0` which will instead be simplified to `true` before we get to canonicalizing the select.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119690