[llvm] r358919 - [InstCombine] Eliminate stores to constant memory

Fri Jul 26 10:34:56 PDT 2019

For those finding this thread, another issue was reported here:
https://bugs.llvm.org/show_bug.cgi?id=42763

On Thu, Apr 25, 2019 at 10:28 AM Chandler Carruth via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
> I wanted to write this so people were aware this patch will break code that exists in the wild.
>
> Consider code like this from BusyBox:
> https://github.com/bigswitch/iods/blob/master/indigo-lb9a/busybox-1.13.3/shell/ash.c#L216
> extern struct globals_misc *const ash_ptr_to_globals_misc;
>
> The code goes on to cast away the const and store to this global:
> https://github.com/bigswitch/iods/blob/master/indigo-lb9a/busybox-1.13.3/shell/ash.c#L241
>
> I believe your patch is entirely correct. I believe Clang is correct to mark this memory as constant. I just wanted people to be aware that code in the wild doesn't abide by these rules.
>
> If we want to do something about it, I believe we should change Clang to not mark *extern* const globals as constant memory. +Richard Smith for confirmation on that approach. Maybe behind a flag or only in certain modes.
>
> So far, this is the only example I have found however, and so it may be sufficiently rare to not warrant doing anything about it. Sending this email in case others are debugging similar issues, hit this revision, and add information that indicates it is not as rare as I am hoping.
>
>
> On Mon, Apr 22, 2019 at 1:26 PM Philip Reames via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>
>> Author: reames
>> Date: Mon Apr 22 13:28:19 2019
>> New Revision: 358919
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=358919&view=rev
>> Log:
>> [InstCombine] Eliminate stores to constant memory
>>
>> If we have a store to a piece of memory which is known constant, then we know the store must be storing back the same value. As a result, the store (or memset, or memmove) must either be down a dead path, or a noop. In either case, it is valid to simply remove the store.
>>
>> The motivating case for this involves a memmove to a buffer which is constant down a path which is dynamically dead.
>>
>> Note that I'm choosing to implement the less aggressive of two possible semantics here. We could simply say that the store *is undefined*, and prune the path. Consensus in the review was that the more aggressive form might be a good follow on change at a later date.
>>
>> Differential Revision: https://reviews.llvm.org/D60659
>>
>>
>> Modified:
>>     llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
>>     llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>>     llvm/trunk/test/Transforms/InstCombine/memcpy.ll
>>     llvm/trunk/test/Transforms/InstCombine/memmove.ll
>>     llvm/trunk/test/Transforms/InstCombine/memset.ll
>>     llvm/trunk/test/Transforms/InstCombine/store.ll
>>
>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=358919&r1=358918&r2=358919&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp (original)
>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp Mon Apr 22 13:28:19 2019
>> @@ -120,6 +120,15 @@ Instruction *InstCombiner::SimplifyAnyMe
>>      return MI;
>>    }
>>
>> +  // If we have a store to a location which is known constant, we can conclude
>> +  // that the store must be storing the constant value (else the memory
>> +  // wouldn't be constant), and this must be a noop.
>> +  if (AA->pointsToConstantMemory(MI->getDest())) {
>> +    // Set the size of the copy to 0, it will be deleted on the next iteration.
>> +    MI->setLength(Constant::getNullValue(MI->getLength()->getType()));
>> +    return MI;
>> +  }
>> +
>>    // If MemCpyInst length is 1/2/4/8 bytes then replace memcpy with
>>    // load/store.
>>    ConstantInt *MemOpLength = dyn_cast<ConstantInt>(MI->getLength());
>> @@ -218,6 +227,15 @@ Instruction *InstCombiner::SimplifyAnyMe
>>      return MI;
>>    }
>>
>> +  // If we have a store to a location which is known constant, we can conclude
>> +  // that the store must be storing the constant value (else the memory
>> +  // wouldn't be constant), and this must be a noop.
>> +  if (AA->pointsToConstantMemory(MI->getDest())) {
>> +    // Set the size of the copy to 0, it will be deleted on the next iteration.
>> +    MI->setLength(Constant::getNullValue(MI->getLength()->getType()));
>> +    return MI;
>> +  }
>> +
>>    // Extract the length and alignment and fill if they are constant.
>>    ConstantInt *LenC = dyn_cast<ConstantInt>(MI->getLength());
>>    ConstantInt *FillC = dyn_cast<ConstantInt>(MI->getValue());
>>
>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp?rev=358919&r1=358918&r2=358919&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp (original)
>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp Mon Apr 22 13:28:19 2019
>> @@ -1438,6 +1438,12 @@ Instruction *InstCombiner::visitStoreIns
>>      }
>>    }
>>
>> +  // If we have a store to a location which is known constant, we can conclude
>> +  // that the store must be storing the constant value (else the memory
>> +  // wouldn't be constant), and this must be a noop.
>> +  if (AA->pointsToConstantMemory(Ptr))
>> +    return eraseInstFromFunction(SI);
>> +
>>    // Do really simple DSE, to catch cases where there are several consecutive
>>    // stores to the same location, separated by a few arithmetic operations. This
>>    // situation often occurs with bitfield accesses.
>>
>> Modified: llvm/trunk/test/Transforms/InstCombine/memcpy.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memcpy.ll?rev=358919&r1=358918&r2=358919&view=diff
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/InstCombine/memcpy.ll (original)
>> +++ llvm/trunk/test/Transforms/InstCombine/memcpy.ll Mon Apr 22 13:28:19 2019
>> @@ -40,7 +40,6 @@ define void @test3(i8* %d, i8* %s) {
>>
>>  define void @memcpy_to_constant(i8* %src) {
>>  ; CHECK-LABEL: @memcpy_to_constant(
>> -; CHECK-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 bitcast (i128* @UnknownConstant to i8*), i8* align 1 [[SRC:%.*]], i32 16, i1 false)
>>  ; CHECK-NEXT:    ret void
>>  ;
>>    %dest = bitcast i128* @UnknownConstant to i8*
>>
>> Modified: llvm/trunk/test/Transforms/InstCombine/memmove.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memmove.ll?rev=358919&r1=358918&r2=358919&view=diff
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/InstCombine/memmove.ll (original)
>> +++ llvm/trunk/test/Transforms/InstCombine/memmove.ll Mon Apr 22 13:28:19 2019
>> @@ -59,7 +59,6 @@ define void @test4(i8* %a) {
>>
>>  define void @memmove_to_constant(i8* %src) {
>>  ; CHECK-LABEL: @memmove_to_constant(
>> -; CHECK-NEXT:    call void @llvm.memmove.p0i8.p0i8.i32(i8* align 4 bitcast (i128* @UnknownConstant to i8*), i8* align 1 [[SRC:%.*]], i32 16, i1 false)
>>  ; CHECK-NEXT:    ret void
>>  ;
>>    %dest = bitcast i128* @UnknownConstant to i8*
>>
>> Modified: llvm/trunk/test/Transforms/InstCombine/memset.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memset.ll?rev=358919&r1=358918&r2=358919&view=diff
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/InstCombine/memset.ll (original)
>> +++ llvm/trunk/test/Transforms/InstCombine/memset.ll Mon Apr 22 13:28:19 2019
>> @@ -26,7 +26,6 @@ define i32 @test([1024 x i8]* %target) {
>>
>>  define void @memset_to_constant() {
>>  ; CHECK-LABEL: @memset_to_constant(
>> -; CHECK-NEXT:    call void @llvm.memset.p0i8.i32(i8* align 4 bitcast (i128* @Unknown to i8*), i8 0, i32 16, i1 false)
>>  ; CHECK-NEXT:    ret void
>>  ;
>>    %p = bitcast i128* @Unknown to i8*
>>
>> Modified: llvm/trunk/test/Transforms/InstCombine/store.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/store.ll?rev=358919&r1=358918&r2=358919&view=diff
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/InstCombine/store.ll (original)
>> +++ llvm/trunk/test/Transforms/InstCombine/store.ll Mon Apr 22 13:28:19 2019
>> @@ -295,7 +295,6 @@ define void @write_back7(i32* %p) {
>>
>>  define void @store_to_constant() {
>>  ; CHECK-LABEL: @store_to_constant(
>> -; CHECK-NEXT:    store i32 0, i32* @Unknown, align 4
>>  ; CHECK-NEXT:    ret void
>>  ;
>>    store i32 0, i32* @Unknown
>>
>>
>> _______________________________________________
>> 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