<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.gmail-pl-k
{mso-style-name:gmail-pl-k;}
span.EmailStyle19
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal">We had a bug reported about busybox getting miscompiled due to the incorrect use of “const” before; see
<a href="https://bugs.llvm.org/show_bug.cgi?id=39919">https://bugs.llvm.org/show_bug.cgi?id=39919</a> . 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.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">-Eli<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b>From:</b> llvm-commits <llvm-commits-bounces@lists.llvm.org>
<b>On Behalf Of </b>Chandler Carruth via llvm-commits<br>
<b>Sent:</b> Thursday, April 25, 2019 10:28 AM<br>
<b>To:</b> Philip Reames <listmail@philipreames.com>; Richard Smith <richard@metafoo.co.uk><br>
<b>Cc:</b> llvm-commits <llvm-commits@lists.llvm.org><br>
<b>Subject:</b> [EXT] Re: [llvm] r358919 - [InstCombine] Eliminate stores to constant memory<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">I wanted to write this so people were aware this patch will break code that exists in the wild.<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Consider code like this from BusyBox:<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><a href="https://github.com/bigswitch/iods/blob/master/indigo-lb9a/busybox-1.13.3/shell/ash.c#L216">https://github.com/bigswitch/iods/blob/master/indigo-lb9a/busybox-1.13.3/shell/ash.c#L216</a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span class="gmail-pl-k"><span style="font-size:9.0pt;font-family:Consolas;color:#D73A49">extern</span></span><span style="font-size:9.0pt;font-family:Consolas;color:#24292E;background:#FFFBDD">
</span><span class="gmail-pl-k"><span style="font-size:9.0pt;font-family:Consolas;color:#D73A49">struct</span></span><span style="font-size:9.0pt;font-family:Consolas;color:#24292E;background:#FFFBDD"> globals_misc *</span><span class="gmail-pl-k"><span style="font-size:9.0pt;font-family:Consolas;color:#D73A49">const</span></span><span style="font-size:9.0pt;font-family:Consolas;color:#24292E;background:#FFFBDD">
ash_ptr_to_globals_misc;</span><o:p></o:p></p>
</div>
<p class="MsoNormal"><br>
The code goes on to cast away the const and store to this global:<o:p></o:p></p>
<div>
<p class="MsoNormal"><a href="https://github.com/bigswitch/iods/blob/master/indigo-lb9a/busybox-1.13.3/shell/ash.c#L241">https://github.com/bigswitch/iods/blob/master/indigo-lb9a/busybox-1.13.3/shell/ash.c#L241</a><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">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.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">If we want to do something about it, I believe we should change Clang to not mark *extern* const globals as constant memory. <a href="mailto:richard@metafoo.co.uk">+Richard Smith</a> for confirmation on that approach. Maybe behind a flag
or only in certain modes.<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">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.<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Mon, Apr 22, 2019 at 1:26 PM Philip Reames via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal">Author: reames<br>
Date: Mon Apr 22 13:28:19 2019<br>
New Revision: 358919<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=358919&view=rev" target="_blank">
http://llvm.org/viewvc/llvm-project?rev=358919&view=rev</a><br>
Log:<br>
[InstCombine] Eliminate stores to constant memory<br>
<br>
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.<br>
<br>
The motivating case for this involves a memmove to a buffer which is constant down a path which is dynamically dead.<br>
<br>
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.<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D60659" target="_blank">
https://reviews.llvm.org/D60659</a><br>
<br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp<br>
llvm/trunk/test/Transforms/InstCombine/memcpy.ll<br>
llvm/trunk/test/Transforms/InstCombine/memmove.ll<br>
llvm/trunk/test/Transforms/InstCombine/memset.ll<br>
llvm/trunk/test/Transforms/InstCombine/store.ll<br>
<br>
Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=358919&r1=358918&r2=358919&view=diff" target="_blank">
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=358919&r1=358918&r2=358919&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp Mon Apr 22 13:28:19 2019<br>
@@ -120,6 +120,15 @@ Instruction *InstCombiner::SimplifyAnyMe<br>
return MI;<br>
}<br>
<br>
+ // If we have a store to a location which is known constant, we can conclude<br>
+ // that the store must be storing the constant value (else the memory<br>
+ // wouldn't be constant), and this must be a noop.<br>
+ if (AA->pointsToConstantMemory(MI->getDest())) {<br>
+ // Set the size of the copy to 0, it will be deleted on the next iteration.<br>
+ MI->setLength(Constant::getNullValue(MI->getLength()->getType()));<br>
+ return MI;<br>
+ }<br>
+<br>
// If MemCpyInst length is 1/2/4/8 bytes then replace memcpy with<br>
// load/store.<br>
ConstantInt *MemOpLength = dyn_cast<ConstantInt>(MI->getLength());<br>
@@ -218,6 +227,15 @@ Instruction *InstCombiner::SimplifyAnyMe<br>
return MI;<br>
}<br>
<br>
+ // If we have a store to a location which is known constant, we can conclude<br>
+ // that the store must be storing the constant value (else the memory<br>
+ // wouldn't be constant), and this must be a noop.<br>
+ if (AA->pointsToConstantMemory(MI->getDest())) {<br>
+ // Set the size of the copy to 0, it will be deleted on the next iteration.<br>
+ MI->setLength(Constant::getNullValue(MI->getLength()->getType()));<br>
+ return MI;<br>
+ }<br>
+<br>
// Extract the length and alignment and fill if they are constant.<br>
ConstantInt *LenC = dyn_cast<ConstantInt>(MI->getLength());<br>
ConstantInt *FillC = dyn_cast<ConstantInt>(MI->getValue());<br>
<br>
Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp?rev=358919&r1=358918&r2=358919&view=diff" target="_blank">
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp?rev=358919&r1=358918&r2=358919&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp Mon Apr 22 13:28:19 2019<br>
@@ -1438,6 +1438,12 @@ Instruction *InstCombiner::visitStoreIns<br>
}<br>
}<br>
<br>
+ // If we have a store to a location which is known constant, we can conclude<br>
+ // that the store must be storing the constant value (else the memory<br>
+ // wouldn't be constant), and this must be a noop.<br>
+ if (AA->pointsToConstantMemory(Ptr))<br>
+ return eraseInstFromFunction(SI);<br>
+<br>
// Do really simple DSE, to catch cases where there are several consecutive<br>
// stores to the same location, separated by a few arithmetic operations. This<br>
// situation often occurs with bitfield accesses.<br>
<br>
Modified: llvm/trunk/test/Transforms/InstCombine/memcpy.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memcpy.ll?rev=358919&r1=358918&r2=358919&view=diff" target="_blank">
http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memcpy.ll?rev=358919&r1=358918&r2=358919&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/InstCombine/memcpy.ll (original)<br>
+++ llvm/trunk/test/Transforms/InstCombine/memcpy.ll Mon Apr 22 13:28:19 2019<br>
@@ -40,7 +40,6 @@ define void @test3(i8* %d, i8* %s) {<br>
<br>
define void @memcpy_to_constant(i8* %src) {<br>
; CHECK-LABEL: @memcpy_to_constant(<br>
-; 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)<br>
; CHECK-NEXT: ret void<br>
;<br>
%dest = bitcast i128* @UnknownConstant to i8*<br>
<br>
Modified: llvm/trunk/test/Transforms/InstCombine/memmove.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memmove.ll?rev=358919&r1=358918&r2=358919&view=diff" target="_blank">
http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memmove.ll?rev=358919&r1=358918&r2=358919&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/InstCombine/memmove.ll (original)<br>
+++ llvm/trunk/test/Transforms/InstCombine/memmove.ll Mon Apr 22 13:28:19 2019<br>
@@ -59,7 +59,6 @@ define void @test4(i8* %a) {<br>
<br>
define void @memmove_to_constant(i8* %src) {<br>
; CHECK-LABEL: @memmove_to_constant(<br>
-; 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)<br>
; CHECK-NEXT: ret void<br>
;<br>
%dest = bitcast i128* @UnknownConstant to i8*<br>
<br>
Modified: llvm/trunk/test/Transforms/InstCombine/memset.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memset.ll?rev=358919&r1=358918&r2=358919&view=diff" target="_blank">
http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memset.ll?rev=358919&r1=358918&r2=358919&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/InstCombine/memset.ll (original)<br>
+++ llvm/trunk/test/Transforms/InstCombine/memset.ll Mon Apr 22 13:28:19 2019<br>
@@ -26,7 +26,6 @@ define i32 @test([1024 x i8]* %target) {<br>
<br>
define void @memset_to_constant() {<br>
; CHECK-LABEL: @memset_to_constant(<br>
-; CHECK-NEXT: call void @llvm.memset.p0i8.i32(i8* align 4 bitcast (i128* @Unknown to i8*), i8 0, i32 16, i1 false)<br>
; CHECK-NEXT: ret void<br>
;<br>
%p = bitcast i128* @Unknown to i8*<br>
<br>
Modified: llvm/trunk/test/Transforms/InstCombine/store.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/store.ll?rev=358919&r1=358918&r2=358919&view=diff" target="_blank">
http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/store.ll?rev=358919&r1=358918&r2=358919&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/InstCombine/store.ll (original)<br>
+++ llvm/trunk/test/Transforms/InstCombine/store.ll Mon Apr 22 13:28:19 2019<br>
@@ -295,7 +295,6 @@ define void @write_back7(i32* %p) {<br>
<br>
define void @store_to_constant() {<br>
; CHECK-LABEL: @store_to_constant(<br>
-; CHECK-NEXT: store i32 0, i32* @Unknown, align 4<br>
; CHECK-NEXT: ret void<br>
;<br>
store i32 0, i32* @Unknown<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><o:p></o:p></p>
</blockquote>
</div>
</div>
</div>
</body>
</html>