[PATCH] D41297: [ThinLTO] Implement summary visualizer

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks, just some minor stuff left.



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:77
+    /// per-module summaries, when module analysis is being run.
+    const GlobalValue *GV = nullptr;
+
----------------
Better to initialize the appropriate union member when this is constructed, based on the value of IsAnalysis. I think that would just be in getOrInsertValuePtr, which could be passed IsAnalysis from its callers. Then init GV to nullptr or Name to "" depending on that flag.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:147
 
-  static bool isEqual(ValueInfo L, ValueInfo R) { return L.Ref == R.Ref; }
-  static unsigned getHashValue(ValueInfo I) { return (uintptr_t)I.Ref; }
+  // We are not supposed to mix ValueInfo(s) with different analysis flag
+  // in a same container.
----------------
Add an assert then that L.isFromAnalysis() == R.isFromAnalysis() ?


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:375
   assert(PSI);
-  ModuleSummaryIndex Index;
+  ModuleSummaryIndex Index(true);
 
----------------
Please update all of these invocations to document the the constant bool param. E.g. "(/*IsPerformingAnalysis=*/true)"


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:214
+    // 0 corresponds to alias edge, 1 to ref edge, 2 to call with unknown hotness, ...
+    Hotness += 2;    
+    static const char *TypeOrHotness[] = {
----------------
I actually meant change the name of parameter Hotness to TypeOrHotness. The array can stay EdgeAttrs. Sorry for not being clear.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:520
         DEBUG(dbgs() << "Live root: " << Entry.first << "\n");
-        Worklist.push_back(ValueInfo(&Entry));
+        Worklist.push_back(ValueInfo(Index.isPerformingAnalysis(), &Entry));
         ++LiveSymbols;
----------------
We should never be computing dead symbols on a partial (per-module) index, so Index.isPerformingAnalysis should always be false. Suggest passing false here directly (with param name in comment).


================
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:1532
 bool LowerTypeTestsModule::runForTesting(Module &M) {
-  ModuleSummaryIndex Summary;
+  ModuleSummaryIndex Summary(true);
 
----------------
Why is this one true? It doesn't seem like we should be marking as InAnalysis. 


https://reviews.llvm.org/D41297