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

[Philip Reames via llvm-commits]

I no longer have the bandwidth to continue this.  I've expressed a 
concern about a latent bug.  If you believe its not worth following up 
on, that's fine.  I don't have the time to do so myself or reverse 
engineer a pass I'm not familiar with to produce a test case.  I defer 
to the two of you to decide on what you want to do here.


On 01/17/2018 05:00 PM, Friedman, Eli wrote:
> 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
>