On Sat, Mar 31, 2012 at 3:34 PM, Matt Beaumont-Gay <matthewbg at google.com>wrote:
> On Sat, Mar 31, 2012 at 06:17, Chandler Carruth <chandlerc at gmail.com>
> wrote:
> > --- llvm/trunk/lib/Transforms/IPO/InlineAlways.cpp (original)
> > +++ llvm/trunk/lib/Transforms/IPO/InlineAlways.cpp Sat Mar 31 08:17:18
> 2012
> > @@ -32,7 +32,6 @@
> >
> > // AlwaysInliner only inlines functions that are mark as "always
> inline".
> > class AlwaysInliner : public Inliner {
> > - InlineCostAnalyzer CA;
> > public:
> > // Use extremely low threshold.
> > AlwaysInliner() : Inliner(ID, -2000000000, /*InsertLifetime*/true) {
> > @@ -43,24 +42,7 @@
> > initializeAlwaysInlinerPass(*PassRegistry::getPassRegistry());
> > }
> > static char ID; // Pass identification, replacement for typeid
> > - InlineCost getInlineCost(CallSite CS) {
> > - Function *Callee = CS.getCalledFunction();
> > - // We assume indirect calls aren't calling an always-inline
> function.
> > - if (!Callee) return InlineCost::getNever();
> > -
> > - // We can't inline calls to external functions.
> > - // FIXME: We shouldn't even get here.
> > - if (Callee->isDeclaration()) return InlineCost::getNever();
> > -
> > - // Return never for anything not marked as always inline.
> > - if (!Callee->hasFnAttr(Attribute::AlwaysInline))
> > - return InlineCost::getNever();
> > -
> > - // We still have to check the inline cost in case there are
> reasons to
> > - // not inline which trump the always-inline attribute such as
> setjmp and
> > - // indirectbr.
> > - return CA.getInlineCost(CS, getInlineThreshold(CS));
> > - }
> > + virtual InlineCost getInlineCost(CallSite CS);
> > virtual bool doFinalization(CallGraph &CG) {
> > return removeDeadFunctions(CG, /*AlwaysInlineOnly=*/true);
> > }
> > @@ -81,9 +63,70 @@
> > return new AlwaysInliner(InsertLifetime);
> > }
> >
> > +/// \brief Minimal filter to detect invalid constructs for inlining.
> > +static bool isInlineViable(Function &F) {
> > + bool ReturnsTwice = F.hasFnAttr(Attribute::ReturnsTwice);
> > + for (Function::iterator BI = F.begin(), BE = F.end(); BI != BE; ++BI)
> {
> > + // Disallow inlining of functions which contain an indirect branch.
> > + if (isa<IndirectBrInst>(BI->getTerminator()))
> > + return false;
> > +
> > + for (BasicBlock::iterator II = BI->begin(), IE = BI->end(); II !=
> IE;
> > + ++II) {
> > + CallSite CS(II);
> > + if (!CS)
> > + continue;
> > +
> > + // Disallow recursive calls.
> > + if (&F == CS.getCalledFunction())
> > + return false;
> > +
> > + // Disallow calls which expose returns-twice to a function not
> previously
> > + // attributed as such.
> > + if (ReturnsTwice && CS.isCall() &&
>
> Er, should be "if (!ReturnsTwice && ...)", no?
>
Rather! Thanks for the catch!
> Also, tests?
>
Well, I had *thought* that this was nicely covered by existing tests, as
this change was supposed to have no observable behavior change, just be
ridiculously simpler... Yet clearly this functionality is hardly tested at
all. I'll try to get enough tests to catch this bug...
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120331/7edf3494/attachment.html>