[llvm] r358919 - [InstCombine] Eliminate stores to constant memory
Eli Friedman via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 25 12:58:22 PDT 2019
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
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<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
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/20190425/cd03e613/attachment.html>
More information about the llvm-commits
mailing list