[PATCH] D70404: [ThinLTO] Always import constants

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Please update the patch description (still talks about forcing read only).

Presumably new flag should be added to llvm::computeLTOCacheKey where it already hashes the read/write only flags.

In D70404#1756261 <https://reviews.llvm.org/D70404#1756261>, @evgeny777 wrote:

> >   Out of curiosity, what problem do you run into when you unify ReadOnly with Constant?
>
> When constant is always treated as `ReadOnly` it is also always internalized. Now if constant 'A' references constant 'B' in initializer you also have to import constant 'B' when constant 'A' is imported. Otherwise there is linker undef,
>  because both A and B are internal. This requires recursive variable import which we currently lack.
>
> The more serious problem however is that if constant is not `unnamed_addr` you can't internalize it at all, because you may end up with different copies of it, each having unique address. So in any case some extra attribute in
>  GlobalVarSummary is needed to indicate if we can internalize ReadOnly var or not.


I see a test for the case where the constant must be imported as promoted, but perhaps there should be one to ensure we can import as internal when possible?



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


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:722
+            bool Constant = false;
+            if (auto *GVar = dyn_cast<GlobalVariable>(GV))
+              Constant = GVar->isConstant();
----------------
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.


================
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
----------------
Looks like it is promoted, not internalized?


================
Comment at: llvm/test/ThinLTO/X86/import-constant.ll:15
+
+; IMPORT:      @outer = internal local_unnamed_addr global %struct.Q zeroinitializer
+; IMPORT-NEXT: @_ZL3Obj.llvm.{{.*}} =  available_externally hidden constant %struct.S { i32 4, i32 8, i32* @val }
----------------
outer is internalized and given zeroinitializer because it is write only I assume - can you add a comment to that effect here?


================
Comment at: llvm/test/ThinLTO/X86/import-constant.ll:16
+; IMPORT:      @outer = internal local_unnamed_addr global %struct.Q zeroinitializer
+; IMPORT-NEXT: @_ZL3Obj.llvm.{{.*}} =  available_externally hidden constant %struct.S { i32 4, i32 8, i32* @val }
+; IMPORT-NEXT: @val = external dso_local global i32
----------------
Check that also promoted in exporting module?


================
Comment at: llvm/test/ThinLTO/X86/referenced_by_constant.ll:7
 ; Check the import side: we currently only import bar() (with a future
 ; enhancement to identify constants in the summary, we should mark
 ; @someglobal/@someglobal2 for import as a local copy, which would
----------------
Looks like this comment should be updated now?


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

https://reviews.llvm.org/D70404