[llvm] 4e413e1 - [InstCombine] Temporarily do not drop volatile stores before unreachable

[Philip Reames via llvm-commits]

On 9/10/20 7:22 AM, Matt Arsenault via llvm-commits wrote:
>
>> On Sep 10, 2020, at 10:18, Nikita Popov via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>
>>
>> Author: Nikita Popov
>> Date: 2020-09-10T16:16:44+02:00
>> New Revision: 4e413e16216d0c94ada2171f3c59e0a85f4fa4b6
>>
>> URL: https://github.com/llvm/llvm-project/commit/4e413e16216d0c94ada2171f3c59e0a85f4fa4b6
>> DIFF: https://github.com/llvm/llvm-project/commit/4e413e16216d0c94ada2171f3c59e0a85f4fa4b6.diff
>>
>> LOG: [InstCombine] Temporarily do not drop volatile stores before unreachable
>>
>> See discussion in D87149. Dropping volatile stores here is legal
>> per LLVM semantics, but causes issues for real code and may result
>> in a change to LLVM volatile semantics. Temporarily treat volatile
>> stores as "not guaranteed to transfer execution" in just this place,
>> until this issue has been resolved.
>>
>> Added:
>>
>>
>> Modified:
>>     llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
>>     llvm/test/Transforms/InstCombine/volatile_store.ll
>>
>> Removed:
>>
>>
>>
>> ################################################################################
>> diff  --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
>> index 0ca256860c59..63ba7eb85c66 100644
>> --- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
>> +++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
>> @@ -2805,6 +2805,14 @@ Instruction *InstCombinerImpl::visitUnreachableInst(UnreachableInst &I) {
>>    Instruction *Prev = I.getPrevNonDebugInstruction();
>>    if (Prev && !Prev->isEHPad() &&
>>        isGuaranteedToTransferExecutionToSuccessor(Prev)) {
>> +    // Temporarily disable removal of volatile stores preceding unreachable,
>> +    // pending a potential LangRef change permitting volatile stores to trap.
>> +    // TODO: Either remove this code, or properly integrate the check into
>> +    // isGuaranteedToTransferExecutionToSuccessor().
>> +    if (auto *SI = dyn_cast<StoreInst>(Prev))
>> +      if (SI->isVolatile())
>> +        return nullptr;
> This misses atomics and volatile memcpy/memset and co. I think there’s a helper somewhere to check all of them

Please do not do this for atomics.  That will break optimization for 
real code I care about.

Having volatiles be not guaranteed to transfer execution might be 
reasonable; the same for atomics is not.

>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits