[llvm] 64af9f6 - [InstSimplify] add 'x + poison -> poison' (needed for NewGVN)

[Philip Reames via llvm-commits]

On 1/2/22 9:49 AM, Nuno Lopes wrote:
> That's a long story..
>
> The return value of InstSimplify is only guaranteed to be a 
> *refinement* of the input, meaning it's not necessarily equivalent. 
> This is perfectly fine. LLVM can transform undef -> 0; it removes 
> behavior.
>
> GVN algorithms, on the other hand, compute classes of equivalent 
> expressions. But NewGVN uses InstSimplify to canonicalize expressions 
> before computing the equivalence classes. But, ouch, there's a 
> mismatch: GVN deals with equivalences, while InstSimplify gives it 
> refinements.
>
> A workaround implemented at the time was to disable some refinements 
> from InstSimplify by disabling the matching of undef. This is not a 
> complete solution, but a band aid (in theory, at least, I didn't fuzz 
> it to check if it's possible to get a miscompilation in practice).
> See https://llvm.org/doxygen/structllvm_1_1SimplifyQuery.html

Ah, gotcha.  The refinement vs equivalence point makes sense.

A couple of suggestions/ideas:

  * The naming in the code should match the refinement terminology. 
    AllowRefinement would be a much clearer name than WithoutUndef.  In
    particular, the "isUndefValue" in the code is confusing.
  * We really need a way to test the logic and fuzz it.  As a
    suggestion, maybe add an off-by-default option to the instsimplify
    pass called "-instsimplify-disallow-refinement", and write LIT cases
    for the interesting differences?
  * Once we had that, using alive2 to check fuzz outputs for unexpected
    refinements would be pretty straight forward.

>
> Note that InstSimplify does have transformations that are wrong in the 
> presence of undef. It applies redistributivity freely, which is not 
> guaranteed to yield a correct result until we kill undef. But again, 
> it disables undef matching when it recurses, so maybe it's ok in 
> practice too (candidate for fuzzing too).
And a great case to cover with some LIT tests.  :)
>
> Nuno
>
>
> -----Original Message----- From: Philip Reames
> Sent: Sunday, January 2, 2022 5:28 PM
> Subject: Re: [llvm] 64af9f6 - [InstSimplify] add 'x + poison -> 
> poison' (needed for NewGVN)
>
> Er, that really stinks that the public API of this utility is broken 
> then.
>
> Can you explain the context of why newgvn needs to "disable matching on
> undef" and instsimplify is fine to run without doing so?  If there's a
> clear comment in the code already, a pointer to that is fine.
>
> Philip
>
> On 1/2/22 9:26 AM, Nuno Lopes wrote:
>> I don't think it can be reproduced with InstSimplify alone. 
>> InstSimplify already simplified the test case I added. NewGVN 
>> disables matching of undef, which InstSimplify does not.
>>
>> Nuno
>>
>>
>> -----Original Message-----
>> From: Philip Reames
>> Sent: Sunday, January 2, 2022 5:10 PM
>> Subject: Re: [llvm] 64af9f6 - [InstSimplify] add 'x + poison -> 
>> poison' (needed for NewGVN)
>>
>> I agree, this really needs a test.  I see you later added a GVN test for
>> the same, can you add an instsimplify one please?
>>
>> Philip
>>
>> On 12/30/21 4:02 AM, Roman Lebedev via llvm-commits wrote:
>>> Test?
>>>
>>> On Thu, Dec 30, 2021 at 2:53 PM Nuno Lopes via llvm-commits
>>> <llvm-commits at lists.llvm.org> wrote:
>>>>
>>>> Author: Nuno Lopes
>>>> Date: 2021-12-30T11:52:42Z
>>>> New Revision: 64af9f61c30191482979c6883e4cc63703f12010
>>>>
>>>> URL: 
>>>> https://github.com/llvm/llvm-project/commit/64af9f61c30191482979c6883e4cc63703f12010
>>>> DIFF: 
>>>> https://github.com/llvm/llvm-project/commit/64af9f61c30191482979c6883e4cc63703f12010.diff
>>>>
>>>> LOG: [InstSimplify] add 'x + poison -> poison' (needed for NewGVN)
>>>>
>>>> Added:
>>>>
>>>>
>>>> Modified:
>>>>      llvm/lib/Analysis/InstructionSimplify.cpp
>>>>
>>>> Removed:
>>>>
>>>>
>>>>
>>>> ################################################################################ 
>>>>
>>>> diff  --git a/llvm/lib/Analysis/InstructionSimplify.cpp 
>>>> b/llvm/lib/Analysis/InstructionSimplify.cpp
>>>> index 1c26ab3619089..4a8dc754349bd 100644
>>>> --- a/llvm/lib/Analysis/InstructionSimplify.cpp
>>>> +++ b/llvm/lib/Analysis/InstructionSimplify.cpp
>>>> @@ -620,6 +620,10 @@ static Value *SimplifyAddInst(Value *Op0, 
>>>> Value *Op1, bool IsNSW, bool IsNUW,
>>>>     if (Constant *C = foldOrCommuteConstant(Instruction::Add, Op0, 
>>>> Op1, Q))
>>>>       return C;
>>>>
>>>> +  // X + poison -> poison
>>>> +  if (isa<PoisonValue>(Op1))
>>>> +    return Op1;
>>>> +
>>>>     // X + undef -> undef
>>>>     if (Q.isUndefValue(Op1))
>>>>       return Op1;
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220102/1362ad18/attachment-0001.html>