[llvm] r358919 - [InstCombine] Eliminate stores to constant memory
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 26 15:32:59 PDT 2019
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.
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?
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> *On Behalf
> Of *Chandler Carruth via llvm-commits
> *Sent:* Thursday, April 25, 2019 10:28 AM
> *To:* Philip Reames <listmail at philipreames.com>; Richard Smith
> <richard at metafoo.co.uk>
> *Cc:* llvm-commits <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
>
> externstructglobals_misc *constash_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
> <mailto: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 <mailto: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
> <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 <mailto: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/f4026bba/attachment.html>
More information about the llvm-commits
mailing list