[PATCH] D63026: [InstCombine] Fold icmp eq/ne (and %x, signbit), 0 -> %x s>=/s< 0 earlier

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1643-1644
 
   if (!And->hasOneUse())
     return nullptr;
 
----------------
huihuiz wrote:
> lebedev.ri wrote:
> > Move after the newly added code, this restriction does not apply for the pattern in question.
> The restriction "And operation having only one use" was introduced in r134379 (resolving PR10267).
> 
> When And operation has more than one use, applying the following folds will transform an equality compare into an inequality compare, without cutting done any extra instructions. Inequalities are generally more expensive.
> 
> This restriction should apply to fold "(X & signbit) ==/!= 0) --> X s>=/s< 0"and "((X & ~C) ==/!= 0) C+1 is power of 2 --> X u</u>= C+1".
> 
> 
Aha, thank you for digging that up.

> The restriction "And operation having only one use" was introduced in r134379 (resolving PR10267).
> When And operation has more than one use, applying the following folds will transform an equality compare into an inequality compare, without cutting done any extra instructions. Inequalities are generally more expensive.

All that (including rL134379) reads wrong to me.
There is no such cost-model in middle-end,
here in instcombine we only care about not increasing instruction count.
If something is worse for backend/target, then the opposite fold should be done in backend.

Not having this one-use restriction does not result in increasing
instruction count, and decreases "the dependency chain" of the `icmp`,
since it no longer depends on `and`, i.e. the restriction is incorrect/harmful.

But i suppose lifting it up first requires a backend fix, so let's leave it for now :/


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63026