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