[llvm] r348181 - [InstCombine] foldICmpWithLowBitMaskedVal(): disable 2 faulty folds.

[Friedman, Eli via llvm-commits]

On 12/4/2018 5:21 AM, Roman Lebedev wrote:
> On Tue, Dec 4, 2018 at 4:26 AM Friedman, Eli <efriedma at codeaurora.org> wrote:
>> On 12/3/2018 12:07 PM, Roman Lebedev via llvm-commits wrote:
>>> Author: lebedevri
>>> Date: Mon Dec  3 12:07:58 2018
>>> New Revision: 348181
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=348181&view=rev
>>> Log:
>>> [InstCombine] foldICmpWithLowBitMaskedVal(): disable 2 faulty folds.
>>>
>>> These two folds are invalid for this non-constant pattern
>>> when the mask ends up being all-ones:
>>> https://rise4fun.com/Alive/9au
>>> https://rise4fun.com/Alive/UcQM
>>>
>>> Fixes https://bugs.llvm.org/show_bug.cgi?id=39861
>>>
>>> Modified:
>>>       llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
>>>       llvm/trunk/test/Transforms/InstCombine/canonicalize-constant-low-bit-mask-and-icmp-sge-to-icmp-sle.ll
>>>       llvm/trunk/test/Transforms/InstCombine/canonicalize-constant-low-bit-mask-and-icmp-slt-to-icmp-sgt.ll
>>>
>>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp?rev=348181&r1=348180&r2=348181&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp (original)
>>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp Mon Dec  3 12:07:58 2018
>>> @@ -2955,12 +2955,16 @@ static Value *foldICmpWithLowBitMaskedVa
>>>        //  x & (-1 >> y) s>= x    ->    x s<= (-1 >> y)
>>>        if (X != I.getOperand(1)) // X must be on RHS of comparison!
>>>          return nullptr;         // Ignore the other case.
>>> +    if (!match(M, m_Constant())) // Can not do this fold with non-constant.
>>> +      return nullptr;
>> Is there some guarantee that the constant can't be -1?
> No. It's an implicit assumption that such a trivial instsimplify fold
> will have always already happened before we ever get here.
> I should have at least added an assert here, i guess?

Not sure why you would expect that, in general, given the various ways 
we can call into InstSimplify.  If there's some reason I'm missing that 
would make it impossible, please add an assert; otherwise please just 
add an early return.

-Eli

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project