<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 10, 2018 at 3:36 PM, Philip Reames via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I think this is wrong. In particular, an ordered memory operation is NOT safe to remove during constant propagation, even if it's result is known. The ordered load has a side effect of ordering other loads which is not safe to remove. The entire *point* of having side effects is that there's an effect other than the result of the instruction.<br>
<br>
Now, looking at the code I see that this is not a bug this change introduced. </blockquote><div><br></div><div>Yes, my only goal was to keep the same semantics that existed prior to this change.</div><div>If we think these semantics are wrong, ...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> It appears to have been present in the code previously. (Well, if tryToReplaceWithConstant could even return true for said side effecting instructions.)<br>
<br>
I'll point out that another call site to the same function exists for the same purpose in the same file. This is clearly not the first time this has come up.<br>
<br>
I am now specifically asking that this be changed to use isInstructionTriviallyDead which accounts properly for these cases, or a clear test case where that is not sufficient be provided.<br>
<br>
Philip<div class="HOEnZb"><div class="h5"><br>
<br>
<br>
On 01/10/2018 07:22 AM, Davide Italiano wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Please refer to the review for details.<br>
Copy/pasting Danny's comment here for your convenience:<br>
<br>
"That would not return true for anything with side effects. So it will<br>
not handle the atomic loads and stores cases. mayHaveSideEffects will<br>
return true for anything that might write to memory. mayWriteToMemory<br>
will return true for any store, and any ordered load.<br>
<br>
In fact, even getModRefInfo wouldn't help you with that here, it would<br>
do the same, and can't be overridden by anything more powerful right<br>
now (we only have an AA interface for callsites, the regular<br>
instructions are all single implementation in AliasAnalysis.cpp)<br>
<br>
So TL; DR if you wanted to use isInstructionTriviallyDead, it would be<br>
something like isInstructionTriviallyDead || is a load || is a store<br>
|| atomicrmw || cmpxchng || ...."<br>
<br>
--<br>
Davide<br>
<br>
On Tue, Jan 9, 2018 at 10:40 PM, Philip Reames<br>
<<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I think I'm missing something then. Why would any trivially dead<br>
instruction not be safe to remove in SCCP after we've done the constant<br>
folding of the result?<br>
<br>
Philip<br>
<br>
<br>
<br>
On 01/09/2018 04:09 PM, Davide Italiano wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
hmm, the original proposal was that of keeping this function private<br>
to SCCP as it has slightly different requirement<br>
(`isInstructionTriviallyDead()<wbr>` wasn't enough in this case).<br>
Anyway, happy either way (if this gets merged as Philip suggested).<br>
<br>
On Tue, Jan 9, 2018 at 4:01 PM, Philip Reames via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This appears to be duplicating functionality from<br>
isInstructionTriviallyDead. Please merge them.<br>
<br>
Philip<br>
<br>
<br>
<br>
On 01/09/2018 01:58 PM, Chris Bieneman via llvm-commits wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: cbieneman<br>
Date: Tue Jan 9 13:58:46 2018<br>
New Revision: 322125<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=322125&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=322125&view=rev</a><br>
Log:<br>
[IPSCCP] Remove calls without side effects<br>
<br>
Summary:<br>
When performing constant propagation for call instructions we have<br>
historically replaced all uses of the return from a call, but not<br>
removed<br>
the call itself. This is required for correctness if the calls have side<br>
effects, however the compiler should be able to safely remove calls that<br>
don't have side effects.<br>
<br>
This allows the compiler to completely fold away calls to functions that<br>
have no side effects if the inputs are constant and the output can be<br>
determined at compile time.<br>
<br>
Reviewers: davide, sanjoy, bruno, dberlin<br>
<br>
Subscribers: llvm-commits<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D38856" rel="noreferrer" target="_blank">https://reviews.llvm.org/D3885<wbr>6</a><br>
<br>
Added:<br>
llvm/trunk/test/Transforms/IP<wbr>ConstantProp/remove-call-inst.<wbr>ll<br>
- copied, changed from r322124,<br>
llvm/trunk/test/Transforms/IPC<wbr>onstantProp/user-with-multiple<wbr>-uses.ll<br>
Modified:<br>
llvm/trunk/include/llvm/IR/In<wbr>struction.h<br>
llvm/trunk/lib/IR/<wbr>Instruction.cpp<br>
llvm/trunk/lib/Transforms/Sca<wbr>lar/SCCP.cpp<br>
<br>
llvm/trunk/test/Transforms/IPC<wbr>onstantProp/user-with-multiple<wbr>-uses.ll<br>
<br>
Modified: llvm/trunk/include/llvm/IR/Ins<wbr>truction.h<br>
URL:<br>
<br>
<a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Instruction.h?rev=322125&r1=322124&r2=322125&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/include/llvm/<wbr>IR/Instruction.h?rev=322125&<wbr>r1=322124&r2=322125&view=diff</a><br>
<br>
<br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/include/llvm/IR/Ins<wbr>truction.h (original)<br>
+++ llvm/trunk/include/llvm/IR/Ins<wbr>truction.h Tue Jan 9 13:58:46 2018<br>
@@ -535,6 +535,14 @@ public:<br>
/// matters, isSafeToSpeculativelyExecute may be more appropriate.<br>
bool mayHaveSideEffects() const { return mayWriteToMemory() ||<br>
mayThrow(); }<br>
+ /// Return true if the instruction can be removed if the result is<br>
unused.<br>
+ ///<br>
+ /// When constant folding some instructions cannot be removed even if<br>
their<br>
+ /// results are unused. Specifically terminator instructions and<br>
calls<br>
that<br>
+ /// may have side effects cannot be removed without semantically<br>
changing the<br>
+ /// generated program.<br>
+ bool isSafeToRemove() const;<br>
+<br>
/// Return true if the instruction is a variety of EH-block.<br>
bool isEHPad() const {<br>
switch (getOpcode()) {<br>
<br>
Modified: llvm/trunk/lib/IR/Instruction.<wbr>cpp<br>
URL:<br>
<br>
<a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Instruction.cpp?rev=322125&r1=322124&r2=322125&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/lib/IR/Instru<wbr>ction.cpp?rev=322125&r1=322124<wbr>&r2=322125&view=diff</a><br>
<br>
<br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/lib/IR/Instruction.<wbr>cpp (original)<br>
+++ llvm/trunk/lib/IR/Instruction.<wbr>cpp Tue Jan 9 13:58:46 2018<br>
@@ -589,6 +589,11 @@ bool Instruction::mayThrow() const {<br>
return isa<ResumeInst>(this);<br>
}<br>
+bool Instruction::isSafeToRemove() const {<br>
+ return (!isa<CallInst>(this) || !this->mayHaveSideEffects()) &&<br>
+ !isa<TerminatorInst>(this);<br>
+}<br>
+<br>
bool Instruction::isAssociative() const {<br>
unsigned Opcode = getOpcode();<br>
if (isAssociative(Opcode))<br>
<br>
Modified: llvm/trunk/lib/Transforms/Scal<wbr>ar/SCCP.cpp<br>
URL:<br>
<br>
<a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/SCCP.cpp?rev=322125&r1=322124&r2=322125&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/lib/Transform<wbr>s/Scalar/SCCP.cpp?rev=322125&<wbr>r1=322124&r2=322125&view=diff</a><br>
<br>
<br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/lib/Transforms/Scal<wbr>ar/SCCP.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Scal<wbr>ar/SCCP.cpp Tue Jan 9 13:58:46 2018<br>
@@ -1900,7 +1900,7 @@ static bool runIPSCCP(Module &M, const D<br>
if (Inst->getType()->isVoidTy())<br>
continue;<br>
if (tryToReplaceWithConstant(Solv<wbr>er, Inst)) {<br>
- if (!isa<CallInst>(Inst) && !isa<TerminatorInst>(Inst))<br>
+ if (Inst->isSafeToRemove())<br>
Inst->eraseFromParent();<br>
// Hey, we just changed something!<br>
MadeChanges = true;<br>
<br>
Copied: llvm/trunk/test/Transforms/IPC<wbr>onstantProp/remove-call-inst.<wbr>ll<br>
(from r322124,<br>
llvm/trunk/test/Transforms/IPC<wbr>onstantProp/user-with-multiple<wbr>-uses.ll)<br>
URL:<br>
<br>
<a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/IPConstantProp/remove-call-inst.ll?p2=llvm/trunk/test/Transforms/IPConstantProp/remove-call-inst.ll&p1=llvm/trunk/test/Transforms/IPConstantProp/user-with-multiple-uses.ll&r1=322124&r2=322125&rev=322125&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/test/Transfor<wbr>ms/IPConstantProp/remove-call-<wbr>inst.ll?p2=llvm/trunk/test/<wbr>Transforms/IPConstantProp/<wbr>remove-call-inst.ll&p1=llvm/<wbr>trunk/test/Transforms/<wbr>IPConstantProp/user-with-<wbr>multiple-uses.ll&r1=322124&r2=<wbr>322125&rev=322125&view=diff</a><br>
<br>
<br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/test/Transforms/IPC<wbr>onstantProp/user-with-multiple<wbr>-uses.ll<br>
(original)<br>
+++ llvm/trunk/test/Transforms/IPC<wbr>onstantProp/remove-call-inst.<wbr>ll Tue<br>
Jan<br>
9 13:58:46 2018<br>
@@ -6,7 +6,7 @@<br>
; CHECK: define i32 @main() #0 {<br>
; CHECK-NEXT: entry:<br>
-; CHECK-NEXT: %call2 = tail call i32 @wwrite(i64 0) [[NUW:#[0-9]+]]<br>
+; CHECK-NOT: call<br>
; CHECK-NEXT: ret i32 123<br>
define i32 @main() noreturn nounwind {<br>
@@ -31,4 +31,3 @@ return:<br>
; CHECK: attributes #0 = { noreturn nounwind }<br>
; CHECK: attributes #1 = { nounwind readnone }<br>
-; CHECK: attributes [[NUW]] = { nounwind }<br>
<br>
Modified:<br>
llvm/trunk/test/Transforms/IPC<wbr>onstantProp/user-with-multiple<wbr>-uses.ll<br>
URL:<br>
<br>
<a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/IPConstantProp/user-with-multiple-uses.ll?rev=322125&r1=322124&r2=322125&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/test/Transfor<wbr>ms/IPConstantProp/user-with-<wbr>multiple-uses.ll?rev=322125&<wbr>r1=322124&r2=322125&view=diff</a><br>
<br>
<br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/test/Transforms/IPC<wbr>onstantProp/user-with-multiple<wbr>-uses.ll<br>
(original)<br>
+++ llvm/trunk/test/Transforms/IPC<wbr>onstantProp/user-with-multiple<wbr>-uses.ll<br>
Tue Jan 9 13:58:46 2018<br>
@@ -15,7 +15,7 @@ entry:<br>
ret i32 %call2<br>
}<br>
-define internal i32 @wwrite(i64 %i) nounwind readnone {<br>
+define internal i32 @wwrite(i64 %i) nounwind {<br>
entry:<br>
switch i64 %i, label %sw.default [<br>
i64 3, label %return<br>
@@ -30,5 +30,4 @@ return:<br>
}<br>
; CHECK: attributes #0 = { noreturn nounwind }<br>
-; CHECK: attributes #1 = { nounwind readnone }<br>
-; CHECK: attributes [[NUW]] = { nounwind }<br>
+; CHECK: attributes #1 = { nounwind }<br>
<br>
<br>
______________________________<wbr>_________________<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="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote>
<br>
______________________________<wbr>_________________<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="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote>
<br>
<br>
</blockquote></blockquote>
<br>
<br>
</blockquote>
<br>
______________________________<wbr>_________________<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="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>