[PATCH] D35702: [LTO][ThinLTO] Use the linker resolutions to mark global values as dso_local.

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

Thanks, I'm glad to see this coming!



================
Comment at: lib/LTO/LTO.cpp:691
+        }
+      }
     }
----------------
This does not seem safe in face of hash collision.


================
Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:223
+      for (auto const &Summary : VI.getSummaryList())
+        if (Summary && Summary->isDSOLocal())
+          GV.setDSOLocal(true);
----------------
You have a null check here, while the same iteration in LTO.cpp does not bother. I think we should be able to skip the null check by construction.


================
Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:225
+          GV.setDSOLocal(true);
+    }
+  }
----------------
This loop is:

```
bool gv_is_dso_local = llvm::any_of(VI.getSummaryList(), [} (const std::unique_ptr<GlobalValueSummary> &Summary) { return Summary->isDSOLocal(); });
if (gv_is_dso_local) 
  GV.setDSOLocal(true);
```


================
Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:228
+
   bool DoPromote = false;
   if (GV.hasLocalLinkage() &&
----------------
I was trying to figure out if your code handles declarations as well, and it seems so.
But interestingly it means that we do the following for declarations as well, I'm not sure it is totally relevant: @tejohnson WDYT? (this is tangential to this review)


Repository:
  rL LLVM

https://reviews.llvm.org/D35702