[PATCH] D14838: [ThinLTO] Metadata linking for imported functions

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Thanks for the comments. Will upload new patch after testing.


================
Comment at: include/llvm/IR/Metadata.h:833
@@ -832,2 +832,3 @@
   /// Once all forward declarations have been resolved, force cycles to be
-  /// resolved.
+  /// resolved. If \a MDMaterialized is true, then any temporary metadata
+  /// is ignored, otherwise it asserts when encountering temporary metadata.
----------------
joker.eph wrote:
> I think we are usually using \p instead of \a when referring to parameters 
Fixed.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3116
@@ +3115,3 @@
+    DenseMap<TrackingMDRef, unsigned> &MDValueToValIDMap, bool OnlyTempMD) {
+  for (unsigned i = 0; i < MDValueList.size(); ++i) {
+    Metadata *MD = MDValueList[i];
----------------
joker.eph wrote:
> I'm reluctant to `i` as a variable name, maybe `unsigned CurrentValueID` instead?
Renamed to ValID.

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3128
@@ +3127,3 @@
+               "Inconsistent metadata value id");
+        return;
+      }
----------------
joker.eph wrote:
> I'm not sure about this early exit?
Good catch, should be a continue. This isn't triggered with current HEAD since it only would affect the case where we import >1 function at a time, which is obviously in progress.

I added another change to set the SavedFwdRefs flag to true whenever we add a forward ref which should allow us to catch any case where we miss saving off any forward references (e.g. if this bug wasn't fixed).

================
Comment at: lib/Linker/LinkModules.cpp:506
@@ -466,1 +505,3 @@
+    if (!shouldLinkMetadata())
+      ValueMapperFlags = ValueMapperFlags | RF_HaveUnmaterializedMetadata;
   }
----------------
joker.eph wrote:
> Suggestion: `ValueMapperFlags |= RF_HaveUnmaterializedMetadata;`
I did that initially because the |= form gives me:

error: assigning to 'llvm::RemapFlags' from incompatible type 'int'

No amount of casting the RF_ value to the enum type gets me past this error. So I have left it as is for now. Suggestions?

================
Comment at: lib/Linker/LinkModules.cpp:994
@@ +993,3 @@
+
+Metadata *ModuleLinker::mapTemporaryMetadata(Metadata *MD) {
+  MDNode *Node = cast<MDNode>(MD);
----------------
joker.eph wrote:
> early return:
> 
> ```
> if (!ValIDToTempMDMap) return nullptr;
> ```
> 
Done

================
Comment at: lib/Linker/LinkModules.cpp:1013
@@ +1012,3 @@
+void ModuleLinker::replaceTemporaryMetadata(const Metadata *OrigMD,
+                                            Metadata *NewMD) {
+#ifndef NDEBUG
----------------
joker.eph wrote:
> same early return: `if (!ValIDToTempMDMap) return;`
Done

================
Comment at: lib/Linker/LinkModules.cpp:2251
@@ -2095,2 +2250,3 @@
   bool RetCode = TheLinker.run();
-  Composite->dropTriviallyDeadConstantArrays();
+  if (!(Flags & Linker::MetadataLinkingPostpass))
+    Composite->dropTriviallyDeadConstantArrays();
----------------
joker.eph wrote:
> Comment?
Done

================
Comment at: tools/llvm-link/llvm-link.cpp:169
@@ -163,2 +168,3 @@
                             Linker &L) {
+  StringMap<DenseMap<unsigned, MDNode *> *> ModuleToTempMDValsMap;
   for (const auto &Import : Imports) {
----------------
joker.eph wrote:
> ```
>  StringMap<std::unique_ptr<DenseMap<unsigned, MDNode *>>> ModuleToTempMDValsMap;
> ```
Good idea, done

================
Comment at: tools/llvm-link/llvm-link.cpp:229
@@ +228,3 @@
+      TempMDVals = ModuleToTempMDValsMap[FileName] =
+          new DenseMap<unsigned, MDNode *>();
+
----------------
joker.eph wrote:
> May I suggest:
> ```
>     auto &TempMDVals = ModuleToTempMDValsMap[FileName];
>     if (!TempMDVals)
>       TempMDVals = new DenseMap<unsigned, MDNode *>();
> ```
> (one query to the map)
> 
Done


http://reviews.llvm.org/D14838