[PATCH] D18986: [ThinLTO] Prevent importing of "llvm.used" values

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

Weird - I didn't get an email with the review comments. I just noticed them because I opened the patch in Phab to ping it.


================
Comment at: include/llvm/IR/Module.h:757
@@ -751,1 +756,3 @@
+                                           bool CompilerUsed);
+
 /// An raw_ostream inserter for modules.
----------------
joker.eph wrote:
> Why was this code-motion needed?
So that it can be used in lib/Analysis/ModuleSummaryAnalysis.cpp

================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:133
@@ +132,3 @@
+      return;
+  }
+
----------------
joker.eph wrote:
> Is it a "big hammer" fix?
> I'd expect to prevent renaming/importing this specific GV, nothing more.
> I see here the same kind of issue as D18298.
Yes, this is a big hammer fix, which will eventually need something more refined. The problem with simply trying to prevent renaming/importing of these specific GVs is the same as the one I noted in D18298: you also need to prevent importing of any references to these GVs (not just the GV themselves), since it is the importing of any reference to them that provokes the need to promote and rename. It is doable, but this is a simple workaround for what should be an uncommon case, especially given that we are currently doing aggressive promotion/renaming in some of the ThinLTO clients. It provides an immediate fix for a correctness problem that can be refined later.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2892
@@ -2890,1 +2891,3 @@
+    return;
+
   Stream.EnterSubblock(bitc::GLOBALVAL_SUMMARY_BLOCK_ID, 3);
----------------
joker.eph wrote:
> Looks like you can go ahead and commit this right now
I can do that.


http://reviews.llvm.org/D18986