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

[Friedman, Eli via llvm-commits]

On 12/4/2018 1:04 PM, Roman Lebedev wrote:
> On Tue, Dec 4, 2018 at 11:59 PM Friedman, Eli <efriedma at codeaurora.org> wrote:
>> 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.
> InstSimplify - yes.
> But this is in instcombine.
> This is *not* run under -instsimplify.

Oh, sorry, got confused.

Anyway, in instcombine, the worklist does not work in any particular 
order, in general; the initial worklist is constructed to visit uses 
after defs, but instruction can be re-added to the worklist in 
essentially arbitrary order.

-Eli

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