[llvm] r322125 - [IPSCCP] Remove calls without side effects

[Friedman, Eli via llvm-commits]

I don't think there's any argument over whether SCCP's behavior is 
correct; at least, nobody has presented any specific issue with the 
current behavior.  (The issue with cmpxchg is theoretical because 
tryToReplaceWithConstant will currently always fail.)

Like I said before, you need to fix wouldInstructionBeTriviallyDead() to 
return true for atomic loads.

-Eli

On 1/17/2018 4:09 PM, Chris Bieneman wrote:
> In working up the patch I noticed it causes a test failure. We have a test to verify that SCCP removes atomic loads and stores. That is a behavior we explicitly test for (see: Transforms/SCCP/atomic-load-store.ll).
>
> The reason I bring this up is because it seems to me that this isn't a pre-existing bug, since we have a test case verifying this behavior it was a deliberate choice to test for these semantics in r138902.
>
> Eli wrote the test case in question, so I think that Philip and Eli need to come to some agreement about what the correct behavior is.
>
> -Chris
>
>> On Jan 11, 2018, at 2:01 PM, Philip Reames via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>
>>
>>
>> On 01/11/2018 12:38 PM, Friedman, Eli wrote:
>>> On 1/10/2018 3:36 PM, Philip Reames via llvm-commits wrote:
>>>> I think this is wrong.  In particular, an ordered memory operation is NOT safe to remove during constant propagation, even if it's result is known.  The ordered load has a side effect of ordering other loads which is not safe to remove.  The entire *point* of having side effects is that there's an effect other than the result of the instruction.
>>> An atomic load with no users is trivially dead.  Per the atomics model, the only "side-effect" of a load is the potential synchronizes-with edge with an earlier store.  But there is no edge unless the load reads a value produced by a "release" store... and you can't prove it does without using the value. wouldInstructionBeTriviallyDead should handle this, but currently doesn't.
>>>
>>> I don't think SCCP tries to fold any other atomic operations.
>>>
>>> -Eli
>>>
>> Let's not get into the details of the concurrency model.  We don't need to understand this bug.  As an example, cmpxchg is plenty. Even if we know the value loaded, we still need to preserve the side effect of the store.  The current code wouldn't, and thus is buggy.
>>
>> Philip
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

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