[PATCH] D70404: [ThinLTO] Always import constants

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 added inline comments.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:604
+  GlobalVarSummary::GVarFlags VarFlags(
+      CanBeInternalized, Constant ? false : CanBeInternalized, Constant);
   auto GVarSummary = std::make_unique<GlobalVarSummary>(Flags, VarFlags,
----------------
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.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:722
+            bool Constant = false;
+            if (auto *GVar = dyn_cast<GlobalVariable>(GV))
+              Constant = GVar->isConstant();
----------------
tejohnson wrote:
> Should this be an assert rather than an if? Looks like we assume it is a GVar since we create a summary for such below. Not sure if def in inline asm can be an alias, but we certainly seem to assume not here.
Yep. It can be just cast<GlobalVariable> instead


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

https://reviews.llvm.org/D70404