[llvm] r277691 - Reinstate "[CloneFunction] Don't remove side effecting calls"
Mikael Holmén via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 22 02:24:03 PDT 2016
Hi,
On 08/19/2016 06:47 PM, David Majnemer wrote:
>
>
> On Fri, Aug 19, 2016 at 12:56 AM, Mikael Holmén
> <mikael.holmen at ericsson.com <mailto:mikael.holmen at ericsson.com>> wrote:
>
> Hi David and Hans,
>
> We've seen a crash with this commit. The below is a failed
> assertion, but with a variant of the input I've also seen a
> segmentation violation.
>
>
> Should be fixed with r279268.
Yes, now I don't see the assertion and I don't get the SEGV for the
other example either.
Great!
/Mikael
>
>
>
> (I don't have a reproducer for the SEGV when compiling for any
> in-tree targets but I can try to get one if it still crashes after
> the assertion problem is fixed.)
>
> opt -S -inline -functionattrs red.ll
>
> gives
>
> opt: ../lib/Analysis/CallGraph.cpp:205: void
> llvm::CallGraphNode::removeCallEdgeFor(llvm::CallSite): Assertion `I
> != CalledFunctions.end() && "Cannot find callsite to remove!"' failed.
> #0 0x0000000001831288 llvm::sys::PrintStackTrace(llvm::raw_ostream&)
> (build-all/bin/opt+0x1831288)
> #1 0x0000000001831e66 SignalHandler(int) (build-all/bin/opt+0x1831e66)
> #2 0x00007f26a8925330 __restore_rt
> (/lib/x86_64-linux-gnu/libpthread.so.0+0x10330)
> #3 0x00007f26a7518c37 gsignal
> /build/eglibc-oGUzwX/eglibc-2.19/signal/../nptl/sysdeps/unix/sysv/linux/raise.c:56:0
> #4 0x00007f26a751c028 abort
> /build/eglibc-oGUzwX/eglibc-2.19/stdlib/abort.c:91:0
> #5 0x00007f26a7511bf6 __assert_fail_base
> /build/eglibc-oGUzwX/eglibc-2.19/assert/assert.c:92:0
> #6 0x00007f26a7511ca2 (/lib/x86_64-linux-gnu/libc.so.6+0x2fca2)
> #7 0x0000000000f6a4de
> llvm::CallGraphNode::removeCallEdgeFor(llvm::CallSite)
> (build-all/bin/opt+0xf6a4de)
> #8 0x000000000186d13a llvm::InlineFunction(llvm::CallSite,
> llvm::InlineFunctionInfo&, llvm::AAResults*, bool)
> (build-all/bin/opt+0x186d13a)
> #9 0x00000000014b4750 inlineCallsImpl(llvm::CallGraphSCC&,
> llvm::CallGraph&, std::function<llvm::AssumptionCache&
> (llvm::Function&)>, llvm::ProfileSummaryInfo*,
> llvm::TargetLibraryInfo&, bool, std::function<llvm::InlineCost
> (llvm::CallSite)>, std::function<llvm::AAResults&
> (llvm::Function&)>, llvm::ImportedFunctionsInliningStatistics&)
> (build-all/bin/opt+0x14b4750)
> #10 0x00000000014b2248
> llvm::Inliner::inlineCalls(llvm::CallGraphSCC&)
> (build-all/bin/opt+0x14b2248)
> #11 0x00000000014b1fd2 llvm::Inliner::runOnSCC(llvm::CallGraphSCC&)
> (build-all/bin/opt+0x14b1fd2)
> #12 0x0000000000f6d823 (anonymous
> namespace)::CGPassManager::runOnModule(llvm::Module&)
> (build-all/bin/opt+0xf6d823)
> #13 0x000000000141ef25
> llvm::legacy::PassManagerImpl::run(llvm::Module&)
> (build-all/bin/opt+0x141ef25)
> #14 0x0000000000667025 main (build-all/bin/opt+0x667025)
> #15 0x00007f26a7503f45 __libc_start_main
> /build/eglibc-oGUzwX/eglibc-2.19/csu/libc-start.c:321:0
> #16 0x0000000000656675 _start (build-all/bin/opt+0x656675)
> Stack dump:
> 0. Program arguments: build-all/bin/opt -S -inline
> -functionattrs red.ll
> 1. Running pass 'CallGraph Pass Manager' on module 'red.ll'.
> Abort
>
> -debug printouts:
>
> Inliner visiting SCC: f1: 0 call sites.
> CGSCCPASSMGR: Refreshing SCC with 1 nodes:
> Call graph node for function: 'f1'<<0x4d327d0>> #uses=2
>
> CGSCCPASSMGR: SCC Refresh didn't change call graph.
> Inliner visiting SCC: f2: 0 call sites.
> CGSCCPASSMGR: Refreshing SCC with 1 nodes:
> Call graph node for function: 'f2'<<0x4d32c60>> #uses=2
>
> CGSCCPASSMGR: SCC Refresh didn't change call graph.
> Inliner visiting SCC: f3: 0 call sites.
> CGSCCPASSMGR: Refreshing SCC with 1 nodes:
> Call graph node for function: 'f3'<<0x4d32d30>> #uses=2
>
> CGSCCPASSMGR: SCC Refresh didn't change call graph.
> Inliner visiting SCC: f4: 3 call sites.
> Analyzing call of f3...
> NumConstantArgs: 0
> NumConstantOffsetPtrArgs: 0
> NumAllocaArgs: 0
> NumConstantPtrCmps: 0
> NumConstantPtrDiffs: 0
> NumInstructionsSimplified: 1
> NumInstructions: 1
> SROACostSavings: 0
> SROACostSavingsLost: 0
> ContainsNoDuplicateCall: 0
> Cost: 0
> Threshold: 337
> Inlining: cost=0, thres=337, Call: call void @f3()
> Analyzing call of f1...
> NumConstantArgs: 1
> NumConstantOffsetPtrArgs: 0
> NumAllocaArgs: 0
> NumConstantPtrCmps: 0
> NumConstantPtrDiffs: 0
> NumInstructionsSimplified: 5
> NumInstructions: 6
> SROACostSavings: 0
> SROACostSavingsLost: 0
> ContainsNoDuplicateCall: 0
> Cost: -5
> Threshold: 225
> Inlining: cost=-5, thres=225, Call: call void @f1(i16 0, i16
> %_tmp29)
> Analyzing call of f2...
> NumConstantArgs: 1
> NumConstantOffsetPtrArgs: 0
> NumAllocaArgs: 0
> NumConstantPtrCmps: 0
> NumConstantPtrDiffs: 0
> NumInstructionsSimplified: 1
> NumInstructions: 1
> SROACostSavings: 0
> SROACostSavingsLost: 0
> ContainsNoDuplicateCall: 0
> Cost: -5
> Threshold: 337
> Inlining: cost=-5, thres=337, Call: %_tmp29 = call i16 @f2(i16 1)
> opt: ../lib/Analysis/CallGraph.cpp:205: void
> llvm::CallGraphNode::removeCallEdgeFor(llvm::CallSite): Assertion `I
> != CalledFunctions.end() && "Cannot find callsite to remove!"' failed.
>
> Regards,
> Mikael
>
>
> On 08/05/2016 12:52 AM, Hans Wennborg via llvm-commits wrote:
>
> Thanks! I grabbed r277773 as well just to be sure :-) Merged in
> r277781.
>
> Since this is close to ToT, please let me know if any problems
> come up
> that require follow-ups.
>
> Cheers,
> Hans
>
> On Thu, Aug 4, 2016 at 10:59 AM, David Majnemer
> <david.majnemer at gmail.com <mailto:david.majnemer at gmail.com>> wrote:
>
> Yes along with r277693.
>
> On Thu, Aug 4, 2016 at 10:13 AM, Hans Wennborg
> <hans at chromium.org <mailto:hans at chromium.org>> wrote:
>
>
> Should we merge this to 3.9?
>
> On Wed, Aug 3, 2016 at 9:24 PM, David Majnemer via
> llvm-commits
> <llvm-commits at lists.llvm.org
> <mailto:llvm-commits at lists.llvm.org>> wrote:
>
> Author: majnemer
> Date: Wed Aug 3 23:24:02 2016
> New Revision: 277691
>
> URL:
> http://llvm.org/viewvc/llvm-project?rev=277691&view=rev
> <http://llvm.org/viewvc/llvm-project?rev=277691&view=rev>
> Log:
> Reinstate "[CloneFunction] Don't remove side
> effecting calls"
>
> This reinstates r277611 + r277614 and reverts
> r277642. A cast_or_null
> should have been a dyn_cast_or_null.
>
> Modified:
> llvm/trunk/lib/Analysis/InstructionSimplify.cpp
> llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp
>
> llvm/trunk/test/Transforms/Inline/inline_constprop.ll
>
> Modified:
> llvm/trunk/lib/Analysis/InstructionSimplify.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/InstructionSimplify.cpp?rev=277691&r1=277690&r2=277691&view=diff
> <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/InstructionSimplify.cpp?rev=277691&r1=277690&r2=277691&view=diff>
>
> ==============================================================================
> --- llvm/trunk/lib/Analysis/InstructionSimplify.cpp
> (original)
> +++ llvm/trunk/lib/Analysis/InstructionSimplify.cpp
> Wed Aug 3 23:24:02
> 2016
> @@ -4356,7 +4356,8 @@ static bool
> replaceAndRecursivelySimplif
>
> // Gracefully handle edge cases where the
> instruction is not wired
> into any
> // parent block.
> - if (I->getParent())
> + if (I->getParent() && !I->isEHPad() &&
> !isa<TerminatorInst>(I) &&
> + !I->mayHaveSideEffects())
> I->eraseFromParent();
> } else {
> Worklist.insert(I);
> @@ -4384,7 +4385,8 @@ static bool
> replaceAndRecursivelySimplif
>
> // Gracefully handle edge cases where the
> instruction is not wired
> into any
> // parent block.
> - if (I->getParent())
> + if (I->getParent() && !I->isEHPad() &&
> !isa<TerminatorInst>(I) &&
> + !I->mayHaveSideEffects())
> I->eraseFromParent();
> }
> return Simplified;
>
> Modified:
> llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp?rev=277691&r1=277690&r2=277691&view=diff
> <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp?rev=277691&r1=277690&r2=277691&view=diff>
>
> ==============================================================================
> ---
> llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp
> (original)
> +++
> llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp
> Wed Aug 3
> 23:24:02 2016
> @@ -14,6 +14,7 @@
>
> //===----------------------------------------------------------------------===//
>
> #include "llvm/Transforms/Utils/Cloning.h"
> +#include "llvm/ADT/SetVector.h"
> #include "llvm/ADT/SmallVector.h"
> #include "llvm/Analysis/ConstantFolding.h"
> #include "llvm/Analysis/InstructionSimplify.h"
> @@ -552,9 +553,39 @@ void
> llvm::CloneAndPruneIntoFromInst(Fun
> // two PHINodes, the iteration over the old PHIs
> remains valid, and
> the
> // mapping will just map us to the new node
> (which may not even be a
> PHI
> // node).
> + const DataLayout &DL =
> NewFunc->getParent()->getDataLayout();
> + SmallSetVector<const Value *, 8> Worklist;
> for (unsigned Idx = 0, Size =
> PHIToResolve.size(); Idx != Size;
> ++Idx)
> - if (PHINode *PN =
> dyn_cast<PHINode>(VMap[PHIToResolve[Idx]]))
> - recursivelySimplifyInstruction(PN);
> + if (isa<PHINode>(VMap[PHIToResolve[Idx]]))
> + Worklist.insert(PHIToResolve[Idx]);
> +
> + // Note that we must test the size on each
> iteration, the worklist
> can grow.
> + for (unsigned Idx = 0; Idx != Worklist.size();
> ++Idx) {
> + const Value *OrigV = Worklist[Idx];
> + auto *I =
> cast_or_null<Instruction>(VMap.lookup(OrigV));
> + if (!I)
> + continue;
> +
> + // See if this instruction simplifies.
> + Value *SimpleV = SimplifyInstruction(I, DL);
> + if (!SimpleV)
> + continue;
> +
> + // Stash away all the uses of the old
> instruction so we can check
> them for
> + // recursive simplifications after a RAUW. This
> is cheaper than
> checking all
> + // uses of To on the recursive step in most cases.
> + for (const User *U : OrigV->users())
> + Worklist.insert(cast<Instruction>(U));
> +
> + // Replace the instruction with its simplified
> value.
> + I->replaceAllUsesWith(SimpleV);
> +
> + // If the original instruction had no side
> effects, remove it.
> + if (isInstructionTriviallyDead(I))
> + I->eraseFromParent();
> + else
> + VMap[OrigV] = I;
> + }
>
> // Now that the inlined function body has been
> fully constructed, go
> through
> // and zap unconditional fall-through branches.
> This happens all the
> time when
>
> Modified:
> llvm/trunk/test/Transforms/Inline/inline_constprop.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/inline_constprop.ll?rev=277691&r1=277690&r2=277691&view=diff
> <http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/inline_constprop.ll?rev=277691&r1=277690&r2=277691&view=diff>
>
> ==============================================================================
> ---
> llvm/trunk/test/Transforms/Inline/inline_constprop.ll (original)
> +++
> llvm/trunk/test/Transforms/Inline/inline_constprop.ll Wed
> Aug 3
> 23:24:02 2016
> @@ -279,3 +279,25 @@ return:
> %retval.0 = phi i32* [ %b, %if.end3 ], [ %a,
> %if.then ]
> ret i32* %retval.0
> }
> +
> +declare i32 @PR28802.external(i32 returned %p1)
> +
> +define internal i32 @PR28802.callee() {
> +entry:
> + br label %cont
> +
> +cont:
> + %0 = phi i32 [ 0, %entry ]
> + %call = call i32 @PR28802.external(i32 %0)
> + ret i32 %call
> +}
> +
> +define i32 @PR28802() {
> +entry:
> + %call = call i32 @PR28802.callee()
> + ret i32 %call
> +}
> +
> +; CHECK-LABEL: define i32 @PR28802(
> +; CHECK: call i32 @PR28802.external(i32 0)
> +; CHECK: ret i32 0
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>
>
More information about the llvm-commits
mailing list