[PATCH] D17028: [ThinLTO] Use MD5 hash in function index.

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:461
@@ -460,1 +460,3 @@
 
+  std::string SourceFileName;
+
----------------
joker.eph wrote:
> doxygen,  at least pointing to the Module one.
Will do.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5461
@@ -5451,3 +5460,3 @@
     case bitc::VST_CODE_FNENTRY: {
       // VST_FNENTRY: [valueid, offset, namechar x N]
       if (convertToString(Record, 2, ValueName))
----------------
joker.eph wrote:
> Typo in the comment: VST_FNENTRY instead of VST_CODE_FNENTRY
Will fix.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5488
@@ -5470,5 +5487,3 @@
     case bitc::VST_CODE_COMBINED_FNENTRY: {
-      // VST_FNENTRY: [offset, namechar x N]
-      if (convertToString(Record, 1, ValueName))
-        return error("Invalid record");
+      // VST_FNENTRY: [offset, funcguid]
       uint64_t FuncSummaryOffset = Record[0];
----------------
joker.eph wrote:
> Typo in the comment: VST_FNENTRY instead of VST_CODE_COMBINED_FNENTRY
> 
Will fix.

================
Comment at: lib/IR/Module.cpp:50
@@ -49,3 +49,3 @@
 Module::Module(StringRef MID, LLVMContext &C)
-    : Context(C), Materializer(), ModuleID(MID), DL("") {
+    : Context(C), Materializer(), ModuleID(MID), SourceFileName(MID), DL("") {
   ValSymTab = new ValueSymbolTable();
----------------
joker.eph wrote:
> Can you initialize `SourceFileName` to empty and test in `getSourceFileName()`?
Actually, this is by design. When we are compiling from source (say clang -c foo.c -flto=thin), we want the SourceFileName to be foo.c. Only when we later compile the foo.o bitcode file do we then want this set to foo.c from the new bitcode record.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:206
@@ -219,1 +205,3 @@
+      // source file name prepended for functions that were originally local
+      // in the source module. Strip any prepended name to recover the original
       // name in the source module.
----------------
joker.eph wrote:
> I was not very happy with that originally, and it may be a good time to revisit it. 
> When does it happen? Why don't we always rename `SrcModule` ?
The old comment was stale. We aren't actually doing any renaming in SrcModule until the importing happens later on. At this point we haven't even materialized everything in SrcModule that we want to import, so I think it is premature to do any renaming.

The CalledFunctionName however comes out of the Worklist which was populated by findExternalCalls. We had to get the global identifier in order to access the correct function summary out of the index here, and to disambiguate from same named locals we are going to import from other modules. 

(Looking at findExternalCalls, I just realized that we should still be checking if this is already in the DestModule using the suffix form, however, since that is the name the local will get when it is promoted. However, with the current bulk importing mechanism we would never already have a promoted local from another module imported into DestModule yet, but I will fix this in case that ever changes.)

================
Comment at: tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp:176
@@ -175,2 +175,3 @@
       STRINGIFY_CODE(MODULE_CODE, METADATA_VALUES)
+      STRINGIFY_CODE(MODULE_CODE, SOURCE_FILENAME)
     }
----------------
joker.eph wrote:
> Test?
Will add one.


http://reviews.llvm.org/D17028