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

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

It looks good to me, with a few minor comments.
I'd rather have another pair of eyes on it since I'm not sure I understand everything though.

Thanks!


================
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.
----------------
I think we are usually using \p instead of \a when referring to parameters 

================
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];
----------------
I'm reluctant to `i` as a variable name, maybe `unsigned CurrentValueID` instead?

================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:3128
@@ +3127,3 @@
+               "Inconsistent metadata value id");
+        return;
+      }
----------------
I'm not sure about this early exit?

================
Comment at: lib/Linker/LinkModules.cpp:506
@@ -466,1 +505,3 @@
+    if (!shouldLinkMetadata())
+      ValueMapperFlags = ValueMapperFlags | RF_HaveUnmaterializedMetadata;
   }
----------------
Suggestion: `ValueMapperFlags |= RF_HaveUnmaterializedMetadata;`

================
Comment at: lib/Linker/LinkModules.cpp:994
@@ +993,3 @@
+
+Metadata *ModuleLinker::mapTemporaryMetadata(Metadata *MD) {
+  MDNode *Node = cast<MDNode>(MD);
----------------
early return:

```
if (!ValIDToTempMDMap) return nullptr;
```


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

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

================
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) {
----------------
```
 StringMap<std::unique_ptr<DenseMap<unsigned, MDNode *>>> ModuleToTempMDValsMap;
```

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



http://reviews.llvm.org/D14838