[PATCH] D70404: [ThinLTO] Always import constants

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Sorry for the delay, I lost track of this one.



================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:604
+  GlobalVarSummary::GVarFlags VarFlags(
+      CanBeInternalized, Constant ? false : CanBeInternalized, Constant);
   auto GVarSummary = std::make_unique<GlobalVarSummary>(Flags, VarFlags,
----------------
evgeny777 wrote:
> tejohnson wrote:
> > Does the change to the write only initialization affect anything user visible? I was trying to reason through it but haven't convinced myself either way. I.e. will it affect which variables are internalized/zeroinitialized?
> I thought it made sense, because no variable can be both constant and writeonly. However setting it to false doesn't really change anything so I removed this.
Ok, true. I'm not sure why I was concerned - I'm ok with the earlier version of the code, agree that it doesn't really make sense to start out with marking it possibly WriteOnly if it cannot be.


================
Comment at: llvm/test/ThinLTO/X86/import-constant.ll:1
+; Check that we internalize constant object in the destination module
+; even when it is referenced in some other GV initializer and/or is
----------------
tejohnson wrote:
> Looks like it is promoted, not internalized?
Looking at this test again, I guess @_ZL3Obj must be getting internalized after promotion, which is why it gets optimized away in the OPT version. Could you check it directly after the internalization phase? I also had a couple other suggestions below.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70404/new/

https://reviews.llvm.org/D70404