<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, Feb 18, 2016 at 1:08 PM Xinliang David Li <<a href="mailto:xinliangli@gmail.com">xinliangli@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Feb 18, 2016 at 12:39 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><span><div dir="ltr">On Thu, Feb 18, 2016 at 12:03 PM Xinliang David Li <<a href="mailto:xinliangli@gmail.com" target="_blank">xinliangli@gmail.com</a>> wrote:<br></div></span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>On Thu, Feb 18, 2016 at 3:03 AM, Chandler Carruth via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:</span><div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
-bool PostOrderFunctionAttrs::runOnSCC(CallGraphSCC &SCC) {<br>
+PreservedAnalyses<br>
+PostOrderFunctionAttrsPass::run(LazyCallGraph::SCC &C, CGSCCAnalysisManager *AM) {<br>
+ Module &M = *C.begin()->getFunction().getParent();<br>
+ const ModuleAnalysisManager &MAM =<br>
+ AM->getResult<ModuleAnalysisManagerCGSCCProxy>(C).getManager();<br>
+ FunctionAnalysisManager &FAM =<br>
+ AM->getResult<FunctionAnalysisManagerCGSCCProxy>(C).getManager();<br>
+<br>
+ // FIXME: Need some way to make it more reasonable to assume that this is<br>
+ // always cached.<br>
+ TargetLibraryInfo &TLI = *MAM.getCachedResult<TargetLibraryAnalysis>(M);<br>
+<br>
+ // We pass a lambda into functions to wire them up to the analysis manager<br>
+ // for getting function analyses.<br>
+ auto AARGetter = [&](Function &F) -> AAResults & {<br>
+ return FAM.getResult<AAManager>(F);<br>
+ };<br>
+<br>
+ // Fill SCCNodes with the elements of the SCC. Also track whether there are<br>
+ // any external or opt-none nodes that will prevent us from optimizing any<br>
+ // part of the SCC.<br>
+ SCCNodeSet SCCNodes;<br>
+ bool HasUnknownCall = false;<br>
+ for (LazyCallGraph::Node &N : C) {<br>
+ Function &F = N.getFunction();<br>
+ if (F.hasFnAttribute(Attribute::OptimizeNone)) {<br>
+ // Treat any function we're trying not to optimize as if it were an<br>
+ // indirect call and omit it from the node set used below.<br>
+ HasUnknownCall = true;<br>
+ continue;<br>
+ }<br>
+ // Track whether any functions in this SCC have an unknown call edge.<br>
+ // Note: if this is ever a performance hit, we can common it with<br>
+ // subsequent routines which also do scans over the instructions of the<br>
+ // function.<br>
+ if (!HasUnknownCall)<br>
+ for (Instruction &I : instructions(F))<br>
+ if (auto CS = CallSite(&I))<br>
+ if (!CS.getCalledFunction()) {<br>
+ HasUnknownCall = true;<br>
+ break;<br>
+ }<br>
+<br></blockquote><div><br></div></div></div></div></div></div><div><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>The above code does not seem to exist in the legacy implementation. Is this a functional change?</div></div></div></div></div></div></blockquote><div><br></div><div>It shouldn't be. Specifically, the legacy pass manager uses the CallGraph analysis rather than the LazyCallGraph analysis. These two analyses provide slightly different models.</div><div><br></div><div>One aspect of the CallGraph analysis is that it puts indirect calls into the call graph where LazyCallGraph doesn't. As a consequence, users of LazyCallGraph need to scan to find them.</div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I see.</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>The difference in the design is motivated by the fact that all the code using the call graph ended up scanning the instructions anyways. There was some discussion both on the lists and elsewhere that motivated me to essentially wait to find a case where it was an important optimization to cache the indirect calls (or calls to declared external functions, they are equally unanalyzable) in the graph for efficiency.</div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>CallGraph basically caches the indirect callsite info in the SCC node. Is it essential to LCG to avoid this design? From this particular case, it seems that the it introduces additional scanning cost (per-function).</div></div></div></div></blockquote><div><br></div><div>I mean, as I said, we considered adding it but decided not to. There is nothing fundamental. While we add a scan here, every routine we're about to call *also* scans all the instructions of all the functions in the SCC, so it seemed unlikely to be a compile time performance problem in practice.</div><div><br></div><div>If this becomes a problem, the first step is to teach function-attrs to do a single scan for all of the attribute inference it wants to do instead of 8+ scans. If it is *still* a problem, then it might make sense to try to cache it somehow.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>thanks,</div><div><br></div><div>David</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><span><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> Also is it possible to refactor the above and share it with the legacy path?</div></div></div></div></blockquote><div><br></div></span><div>I think it would make the code substantially less clear because the two analysis provide different models. I'd rather be able to have the code separate and separately commented precisely to help us ask these kinds of questions. =]</div><div><div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div><br></div><div>thanks,</div><div><br></div><div>David</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ SCCNodes.insert(&F);<br>
+ }<br>
+<br>
+ bool Changed = false;<br>
+ Changed |= addReadAttrs(SCCNodes, AARGetter);<br>
+ Changed |= addArgumentAttrs(SCCNodes);<br>
+<br>
+ // If we have no external nodes participating in the SCC, we can deduce some<br>
+ // more precise attributes as well.<br>
+ if (!HasUnknownCall) {<br>
+ Changed |= addNoAliasAttrs(SCCNodes);<br>
+ Changed |= addNonNullAttrs(SCCNodes, TLI);<br>
+ Changed |= removeConvergentAttrs(SCCNodes);<br>
+ Changed |= addNoRecurseAttrs(SCCNodes);<br>
+ }<br>
+<br>
+ return Changed ? PreservedAnalyses::none() : PreservedAnalyses::all();<br>
+}<br>
+<br>
+namespace {<br>
+struct PostOrderFunctionAttrsLegacyPass : public CallGraphSCCPass {<br>
+ static char ID; // Pass identification, replacement for typeid<br>
+ PostOrderFunctionAttrsLegacyPass() : CallGraphSCCPass(ID) {<br>
+ initializePostOrderFunctionAttrsLegacyPassPass(*PassRegistry::getPassRegistry());<br>
+ }<br>
+<br>
+ bool runOnSCC(CallGraphSCC &SCC) override;<br>
+<br>
+ void getAnalysisUsage(AnalysisUsage &AU) const override {<br>
+ AU.setPreservesCFG();<br>
+ AU.addRequired<AssumptionCacheTracker>();<br>
+ AU.addRequired<TargetLibraryInfoWrapperPass>();<br>
+ addUsedAAAnalyses(AU);<br>
+ CallGraphSCCPass::getAnalysisUsage(AU);<br>
+ }<br>
+<br>
+private:<br>
+ TargetLibraryInfo *TLI;<br>
+};<br>
+}<br>
+<br>
+char PostOrderFunctionAttrsLegacyPass::ID = 0;<br>
+INITIALIZE_PASS_BEGIN(PostOrderFunctionAttrsLegacyPass, "functionattrs",<br>
+ "Deduce function attributes", false, false)<br>
+INITIALIZE_PASS_DEPENDENCY(AssumptionCacheTracker)<br>
+INITIALIZE_PASS_DEPENDENCY(CallGraphWrapperPass)<br>
+INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfoWrapperPass)<br>
+INITIALIZE_PASS_END(PostOrderFunctionAttrsLegacyPass, "functionattrs",<br>
+ "Deduce function attributes", false, false)<br>
+<br>
+Pass *llvm::createPostOrderFunctionAttrsLegacyPass() { return new PostOrderFunctionAttrsLegacyPass(); }<br>
+<br>
+bool PostOrderFunctionAttrsLegacyPass::runOnSCC(CallGraphSCC &SCC) {<br>
TLI = &getAnalysis<TargetLibraryInfoWrapperPass>().getTLI();<br>
bool Changed = false;<br>
<br>
<br>
Modified: llvm/trunk/lib/Transforms/IPO/IPO.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/IPO.cpp?rev=261203&r1=261202&r2=261203&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/IPO.cpp?rev=261203&r1=261202&r2=261203&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/IPO/IPO.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/IPO/IPO.cpp Thu Feb 18 05:03:11 2016<br>
@@ -18,6 +18,7 @@<br>
#include "llvm/InitializePasses.h"<br>
#include "llvm/IR/LegacyPassManager.h"<br>
#include "llvm/Transforms/IPO.h"<br>
+#include "llvm/Transforms/IPO/FunctionAttrs.h"<br>
<br>
using namespace llvm;<br>
<br>
@@ -41,7 +42,7 @@ void llvm::initializeIPO(PassRegistry &R<br>
initializeLowerBitSetsPass(Registry);<br>
initializeMergeFunctionsPass(Registry);<br>
initializePartialInlinerPass(Registry);<br>
- initializePostOrderFunctionAttrsPass(Registry);<br>
+ initializePostOrderFunctionAttrsLegacyPassPass(Registry);<br>
initializeReversePostOrderFunctionAttrsPass(Registry);<br>
initializePruneEHPass(Registry);<br>
initializeStripDeadPrototypesLegacyPassPass(Registry);<br>
@@ -73,7 +74,7 @@ void LLVMAddDeadArgEliminationPass(LLVMP<br>
}<br>
<br>
void LLVMAddFunctionAttrsPass(LLVMPassManagerRef PM) {<br>
- unwrap(PM)->add(createPostOrderFunctionAttrsPass());<br>
+ unwrap(PM)->add(createPostOrderFunctionAttrsLegacyPass());<br>
}<br>
<br>
void LLVMAddFunctionInliningPass(LLVMPassManagerRef PM) {<br>
<br>
Modified: llvm/trunk/lib/Transforms/IPO/PassManagerBuilder.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/PassManagerBuilder.cpp?rev=261203&r1=261202&r2=261203&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/PassManagerBuilder.cpp?rev=261203&r1=261202&r2=261203&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/IPO/PassManagerBuilder.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/IPO/PassManagerBuilder.cpp Thu Feb 18 05:03:11 2016<br>
@@ -31,6 +31,7 @@<br>
#include "llvm/Target/TargetMachine.h"<br>
#include "llvm/Transforms/IPO.h"<br>
#include "llvm/Transforms/IPO/ForceFunctionAttrs.h"<br>
+#include "llvm/Transforms/IPO/FunctionAttrs.h"<br>
#include "llvm/Transforms/IPO/InferFunctionAttrs.h"<br>
#include "llvm/Transforms/Scalar.h"<br>
#include "llvm/Transforms/Vectorize.h"<br>
@@ -382,7 +383,7 @@ void PassManagerBuilder::populateModuleP<br>
Inliner = nullptr;<br>
}<br>
if (!DisableUnitAtATime)<br>
- MPM.add(createPostOrderFunctionAttrsPass());<br>
+ MPM.add(createPostOrderFunctionAttrsLegacyPass());<br>
if (OptLevel > 2)<br>
MPM.add(createArgumentPromotionPass()); // Scalarize uninlined fn args<br>
<br>
@@ -577,7 +578,7 @@ void PassManagerBuilder::addLTOOptimizat<br>
PM.add(createIPSCCPPass());<br>
<br>
// Now that we internalized some globals, see if we can hack on them!<br>
- PM.add(createPostOrderFunctionAttrsPass());<br>
+ PM.add(createPostOrderFunctionAttrsLegacyPass());<br>
PM.add(createReversePostOrderFunctionAttrsPass());<br>
PM.add(createGlobalOptimizerPass());<br>
// Promote any localized global vars.<br>
@@ -627,7 +628,7 @@ void PassManagerBuilder::addLTOOptimizat<br>
PM.add(createScalarReplAggregatesPass());<br>
<br>
// Run a few AA driven optimizations here and now, to cleanup the code.<br>
- PM.add(createPostOrderFunctionAttrsPass()); // Add nocapture.<br>
+ PM.add(createPostOrderFunctionAttrsLegacyPass()); // Add nocapture.<br>
PM.add(createGlobalsAAWrapperPass()); // IP alias analysis.<br>
<br>
PM.add(createLICMPass()); // Hoist loop invariants.<br>
<br>
Modified: llvm/trunk/test/Transforms/FunctionAttrs/readattrs.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/readattrs.ll?rev=261203&r1=261202&r2=261203&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/readattrs.ll?rev=261203&r1=261202&r2=261203&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/FunctionAttrs/readattrs.ll (original)<br>
+++ llvm/trunk/test/Transforms/FunctionAttrs/readattrs.ll Thu Feb 18 05:03:11 2016<br>
@@ -1,4 +1,5 @@<br>
; RUN: opt < %s -functionattrs -S | FileCheck %s<br>
+; RUN: opt < %s -aa-pipeline=basic-aa -passes='require<targetlibinfo>,cgscc(function-attrs)' -S | FileCheck %s<br>
@x = global i32 0<br>
<br>
declare void @test1_1(i8* %x1_1, i8* readonly %y1_1, ...)<br>
<br>
<br>
_______________________________________________<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/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></div></blockquote></div></div></div></div>
</blockquote></div></div></div></blockquote></div></div>