[PATCH] D17859: [InstCombine] convert 'isPositive' and 'isNegative' vector comparisons to shifts (PR26701, PR26819)

[Mehdi Amini via llvm-commits]

> On Mar 24, 2016, at 5:21 PM, Sanjay Patel <spatel at rotateright.com> wrote:
> 
> spatel added inline comments.
> 
> ================
> Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:985
> @@ +984,3 @@
> +        // because vector ISAs may not have a full range of comparison
> +        // operators. This cmp+sext transform, however, will simplify the IR, so
> +        // we always do it.
> ----------------
> joker.eph wrote:
>> I usually see canonicalization as something that is target independent, and then after optimization the lowering can adjust toward something that the target will "like" (ultimately the SDAG legalizer is doing this). Any comment?
> That sounds right to me in general. But are you suggesting that we should do the comparison canonicalizations for vector types too? Ie, (x <=s -1) --> (x <s 0).

Yes.

> 
> My code comment here is my assumption about why we *don't* do that - vector ISAs may have limited functionality, so some backends (x86 at least) would have to then undo that canonicalization.
> 
> I see 2 reasons to justify this patch as written:
> 1. The shift transform is an actual IR optimization, not just a canonicalization - it reduces the instruction count and/or complexity of the IR.

I'm fine with the optimization.

> 2. We're already doing this (shift) transform for some comparisons, so we might as well include the other, related comparison operators where it can apply.

My question was rather "if the canonicalization would be done on vector, would you have to handle all the variant of the comparison operators?"

-- 
Mehdi