[PATCH] D18836: Make alias explicit in the module summary

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: include/llvm/Bitcode/LLVMBitCodes.h:215
@@ +214,3 @@
+  FS_ALIAS = 7,
+  // ALIAS: [modid, linkage, offset]
+  FS_COMBINED_ALIAS = 8,
----------------
COMBINED_ALIAS

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5857
@@ +5856,3 @@
+      if (!AliaseeInfo->summary())
+        return error("Alias expects aliasee summary to be parsed");
+      AS->setAliasee(AliaseeInfo->summary());
----------------
Note somewhere that it is expected that all alias records should follow all of the other summary records?

Also, I think this will be an issue for anonymous functions since won't get a summary for them (see comment below in bitcode writer)

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2928
@@ -2919,3 +2927,3 @@
     // Skip anonymous functions. We will emit a function summary for
     // any aliases below.
     if (!F.hasName())
----------------
Comment is wrong now. With this change we will no longer get a summary emitted for anonymous functions. 

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2964
@@ -2958,3 +2963,3 @@
 static void WriteCombinedGlobalValueSummary(
-    const ModuleSummaryIndex &I, BitstreamWriter &Stream,
+    const ModuleSummaryIndex &Index, BitstreamWriter &Stream,
     std::map<GlobalValue::GUID, unsigned> &GUIDToValueIdMap,
----------------
Do this renaming in a different NFC commit?

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:3120
@@ +3119,3 @@
+    NameVals.push_back(getEncodedLinkage(AS->linkage()));
+    auto AliaseeID = SummaryToOffsetMap[&AS->getAliasee()];
+    assert(AliaseeID);
----------------
The name seems off - this is an offset not an ID.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:3132
@@ +3131,3 @@
+    NameVals.clear();
+    continue;
+  }
----------------
continue is unnecessary

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:90
@@ +89,3 @@
+        if (auto *AS = dyn_cast<AliasSummary>(GVSummary))
+          GVSummary = &AS->getAliasee();
+        auto *Summary = cast<FunctionSummary>(GVSummary);
----------------
Anonymous function may not have summary, see earlier comment

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:347
@@ -346,3 @@
-        // linkOnceODR the linkage does not change. So we import the alias
-        // and aliasee only in this case.
-        const GlobalObject *GO = GV.getBaseObject();
----------------
Lost this comment, I think it was useful.

================
Comment at: test/Bitcode/thinlto-function-summary.ll:10
@@ -9,3 +9,3 @@
 ; BC-NEXT: <PERMODULE {{.*}} op0=2 op1=0
-; BC-NEXT: <PERMODULE {{.*}} op0=4 op1=3
+; BC-NEXT: <ALIAS {{.*}} op0=4 op1=0 op2=3
 ; BC-NEXT: </GLOBALVAL_SUMMARY_BLOCK
----------------
This is a case where aliasee is an anonymous function and doesn't have a summary. I would expect the reading side to hit some of the asserts I called out earlier.


http://reviews.llvm.org/D18836