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

Fri Apr 26 20:24:37 PDT 2019

On Fri, Apr 26, 2019 at 3:33 PM Philip Reames <listmail at philipreames.com>
wrote:

> This doesn't feel like a case where we should change clang's behavior to
> be more conservative by default.  Maybe adding a "const sanitizer" or an
> -fallow-const-misuse might be justified, but I'm not sure I'd go any
> further that that.
>
FWIW, we just patched busybox to not have the const.

As stated originally, I sent this mostly in case many more folks hit these
issues and want to push for a more fundamental strategy. So far I've not
seen much if any cause.

>
>
> If we did want to sanitize for such inputs, replacing the instcombine rule
> I added with an assertion of the same assumption would be pretty
> straight-forward.  I haven't worked on the sanatizers, anyone know if
> there's a natural place to fit such a check?
>
If UBSan doesn't always catch this, please file a bug. It should.

This is a UBSan-style thing -- it might require some minor help from LLVM
but generally should be a trivial and local check.

> Philip
> On 4/25/19 12:58 PM, Eli Friedman wrote:
>
> We had a bug reported about busybox getting miscompiled due to the
> incorrect use of “const” before; see
> https://bugs.llvm.org/show_bug.cgi?id=39919 .  In that case, we weren’t
> removing the store; we just rescheduled it after a load from the same
> variable.  The comments there indicate a bug was reported against busybox,
> but I don’t know what happened after that.
>
>
>
> -Eli
>
>
>
> *From:* llvm-commits <llvm-commits-bounces at lists.llvm.org>
> <llvm-commits-bounces at lists.llvm.org> *On Behalf Of *Chandler Carruth via
> llvm-commits
> *Sent:* Thursday, April 25, 2019 10:28 AM
> *To:* Philip Reames <listmail at philipreames.com>
> <listmail at philipreames.com>; Richard Smith <richard at metafoo.co.uk>
> <richard at metafoo.co.uk>
> *Cc:* llvm-commits <llvm-commits at lists.llvm.org>
> <llvm-commits at lists.llvm.org>
> *Subject:* [EXT] Re: [llvm] r358919 - [InstCombine] Eliminate stores to
> constant memory
>
>
>
> 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
> <richard at metafoo.co.uk> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190426/e410c71a/attachment.html>