[llvm] r305245 - Inliner: Don't remove calls to readnone+nounwind (but not always_inline) functions in the AlwaysInliner
Björn Pettersson A via llvm-commits
llvm-commits at lists.llvm.org
Fri Jun 16 08:47:50 PDT 2017
I’ve analysed this a little bit more.
In CGPassManager::RunPassOnSCC we have this code:
…
Changed |= FPP->runOnFunction(*F);
…
// The function pass(es) modified the IR, they may have clobbered the
// callgraph.
if (Changed && CallGraphUpToDate) {
DEBUG(dbgs() << "CGSCCPASSMGR: Pass Dirtied SCC: "
<< P->getPassName() << '\n');
CallGraphUpToDate = false;
}
But the thing is that even though InstCombine is doing changes (DCE:ing a call), it won’t return true. So the CGPassManager will not set CallGraphUpToDate = false.
(well, I know that InstCombine also says that it preserves CFG… but looks like CGPassManager doesn’t trust function passes when running them this way)
What I noticed was that AddReachableCodeToWorklist (in InstructionCombining.cpp) does not always set MadeIRChange = true even if optimisations are done.
This happens at least for the
// DCE instruction if trivially dead.
and the
// ConstantProp instruction if trivially constant.
optimisations.
If I add
MadeIRChange = true;
when doing those optimisations, then the CGPassManager will set CallGraphUpToDate = false, and later refresh the CallGraph if needed again.
So with such a modification to AddReachableCodeToWorklist we no longer get the assert.
I can prepare a patch for this next week if it sounds reasonable (and no one else is faster than me… my work week has actually ended already..).
Regards,
Björn
From: David Blaikie [mailto:dblaikie at gmail.com]
Sent: den 16 juni 2017 02:17
To: Björn Pettersson A <bjorn.a.pettersson at ericsson.com>; Mikael Holmén <mikael.holmen at ericsson.com>; llvm-commits at lists.llvm.org; Chandler Carruth <chandlerc at gmail.com>
Subject: Re: [llvm] r305245 - Inliner: Don't remove calls to readnone+nounwind (but not always_inline) functions in the AlwaysInliner
Looks like this has exposed a bug in instcombine where instcombine will remove a trivially dead call which can invalidate the call graph (yet instcombine claims to preserve the call graph).
A trivial change to avoid removing such calls seems to be tripping up some things for me... I think it's because a bunch of instcombine at least relies on removing trivially dead calls to intrinsics, perhaps.
hrm...
On Thu, Jun 15, 2017 at 6:40 AM Björn Pettersson A <bjorn.a.pettersson at ericsson.com<mailto:bjorn.a.pettersson at ericsson.com>> wrote:
A smaller reproducer:
;-------------------------------------------------------------------------------
; opt -S -functionattrs -inline -instcombine -o -
; Function Attrs: noinline norecurse nounwind readnone
define void @getPageAddr() #0 {
entry:
%call = call i32 @cmAddr()
ret void
}
; Function Attrs: noinline norecurse nounwind readnone
define internal i32 @cmAddr() #0 {
entry:
ret i32 5
}
attributes #0 = { noinline norecurse nounwind readnone }
;-------------------------------------------------------------------------------
Note that if I execute this using
opt -S -functionattrs -inline -print-callgraph -instcombine -o -
instead of
opt -S -functionattrs -inline -instcombine -o -
the assert won't trigger (although, cmAddr is not removed and I think it asserts when trying to remove that function).
Regards,
Björn
> -----Original Message-----
> From: Mikael Holmén
> Sent: den 15 juni 2017 13:02
> To: David Blaikie <dblaikie at gmail.com<mailto:dblaikie at gmail.com>>; llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>
> Cc: Björn Pettersson A <bjorn.a.pettersson at ericsson.com<mailto:bjorn.a.pettersson at ericsson.com>>
> Subject: Re: [llvm] r305245 - Inliner: Don't remove calls to
> readnone+nounwind (but not always_inline) functions in the AlwaysInliner
>
> Hi David,
>
> With this commit, opt crashes when doing:
>
> opt -S -functionattrs -inline -instcombine -o - callgraph.ll
>
> opt: ../include/llvm/Analysis/CallGraph.h:174:
> llvm::CallGraphNode::~CallGraphNode(): Assertion `NumReferences == 0 &&
> "Node deleted while references remain"' failed.
>
> Regards,
> Mikael
>
> On 06/13/2017 01:01 AM, David Blaikie via llvm-commits wrote:
> > Author: dblaikie
> > Date: Mon Jun 12 18:01:17 2017
> > New Revision: 305245
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=305245&view=rev
> > Log:
> > Inliner: Don't remove calls to readnone+nounwind (but not always_inline)
> functions in the AlwaysInliner
> >
> > Modified:
> > llvm/trunk/lib/Transforms/IPO/Inliner.cpp
> > llvm/trunk/test/Transforms/Inline/always-inline.ll
> >
> > Modified: llvm/trunk/lib/Transforms/IPO/Inliner.cpp
> > URL: http://llvm.org/viewvc/llvm-
> project/llvm/trunk/lib/Transforms/IPO/Inliner.cpp?rev=305245&r1=305244&
> r2=305245&view=diff
> >
> ==========================================================
> ====================
> > --- llvm/trunk/lib/Transforms/IPO/Inliner.cpp (original)
> > +++ llvm/trunk/lib/Transforms/IPO/Inliner.cpp Mon Jun 12 18:01:17 2017
> > @@ -523,6 +523,16 @@ inlineCallsImpl(CallGraphSCC &SCC, CallG
> > if (!Callee || Callee->isDeclaration())
> > continue;
> >
> > + // FIXME for new PM: because of the old PM we currently generate
> ORE and
> > + // in turn BFI on demand. With the new PM, the ORE dependency
> should
> > + // just become a regular analysis dependency.
> > + OptimizationRemarkEmitter ORE(Caller);
> > +
> > + // If the policy determines that we should inline this function,
> > + // delete the call instead.
> > + if (!shouldInline(CS, GetInlineCost, ORE))
> > + continue;
> > +
> > // If this call site is dead and it is to a readonly function, we should
> > // just delete the call instead of trying to inline it, regardless of
> > // size. This happens because IPSCCP propagates the result out of the
> > @@ -548,15 +558,6 @@ inlineCallsImpl(CallGraphSCC &SCC, CallG
> > // Get DebugLoc to report. CS will be invalid after Inliner.
> > DebugLoc DLoc = CS.getInstruction()->getDebugLoc();
> > BasicBlock *Block = CS.getParent();
> > - // FIXME for new PM: because of the old PM we currently generate
> ORE and
> > - // in turn BFI on demand. With the new PM, the ORE dependency
> should
> > - // just become a regular analysis dependency.
> > - OptimizationRemarkEmitter ORE(Caller);
> > -
> > - // If the policy determines that we should inline this function,
> > - // try to do so.
> > - if (!shouldInline(CS, GetInlineCost, ORE))
> > - continue;
> >
> > // Attempt to inline the function.
> > using namespace ore;
> >
> > Modified: llvm/trunk/test/Transforms/Inline/always-inline.ll
> > URL: http://llvm.org/viewvc/llvm-
> project/llvm/trunk/test/Transforms/Inline/always-
> inline.ll?rev=305245&r1=305244&r2=305245&view=diff
> >
> ==========================================================
> ====================
> > --- llvm/trunk/test/Transforms/Inline/always-inline.ll (original)
> > +++ llvm/trunk/test/Transforms/Inline/always-inline.ll Mon Jun 12 18:01:17
> 2017
> > @@ -305,3 +305,14 @@ entry:
> > ret void
> > ; CHECK: ret void
> > }
> > +
> > +define void @inner14() readnone nounwind {
> > +; CHECK: define void @inner14
> > + ret void
> > +}
> > +
> > +define void @outer14() {
> > +; CHECK: call void @inner14
> > + call void @inner14()
> > + ret void
> > +}
> >
> >
> > _______________________________________________
> > 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
> >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170616/35af8ccc/attachment.html>
More information about the llvm-commits
mailing list