[PATCH] D69561: [ThinLTO] Import readonly vars with refs

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 marked 3 inline comments as done.
evgeny777 added inline comments.


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:201
+    // permit import.
+    return WithAttributePropagation && !isReadOnly(GVS) && GVS->refs().size();
+  };
----------------
tejohnson wrote:
> I had forgotten that this was called during attribute propagation. Won't guarding this check with WithAttributePropagation do the wrong thing during importing if we somehow skipped attribute propagation (doesn't look like that can happen today, but let's say we add a debugging option in the future to allow that to be skipped). It would be safer to have this method take a flag that indicates whether we are doing this during attribute propagation, and if not then return true if !WithAttributePropagation.
Makes sense.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:865
+  // This function can be run twice: once with empty list of preserved
+  // symbols and second time when some preserved symbols are given.
+  // We still need to decide if we have to import global variable correctly
----------------
tejohnson wrote:
> Where/how does this happen? There are two callsites, with one using an empty set of preserved symbols (old LTO API), but they are for the old and new LTO APIs, and we would never be calling both.
> 
> It does look like this can potentially be called multiple times when testing through llvm-lto (because the testing facilities there like crossModuleImport(), internalize(), etc) will be called once per input file, but I don't see it otherwise being called twice. If this is just because of llvm-lto testing, can you update the comment to indicate that this is called multiple times when testing via the old LTO API (I don't see it happening via the new LTO API, but maybe I am missing something).
computeDeadSymbols is currently called multiple times from llvm-lto with empty GUIDPreservedSymbols (see `emit_imports.ll`) and can *potemtially* be called with non-empty GUIDPreservedSymbols second time (although *currently* this doesn't happen). However if we add some boolean flag, like `IsCalledFromPropagation` to canImportGlobalVar then this doesn't really matter much.


================
Comment at: test/ThinLTO/X86/local_name_conflict.ll:15
 ; RUN: llvm-lto -thinlto-action=import %t.bc -thinlto-index=%t4.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=IMPORT
-; IMPORT: @baz.llvm.[[HASH:[0-9]+]] = available_externally hidden constant i32 10, align 4
+; IMPORT: @baz.llvm.[[HASH:[0-9]+]] = internal constant i32 10, align 4
 ; IMPORT: call i32 @foo.llvm.[[HASH]]
----------------
tejohnson wrote:
> I assume this is what you were referring to in your comments about the change to processGlobalForThinLTO for cases when actual dead symbol computation was skipped for some reason, but I don't see why we would be skipping the dead symbol computation here - llvm-lto's import action invokes the old LTO crossModuleImport() which ultimately calls computeDeadSymbolsWithConstProp, and I'm not sure why that would do the attribute propagation but not the dead symbol computation.
This is because on some occasions WithGlobalValueDeadStripping can be `false` and WithAttibutePropagation can be `true`. One of such cases is when GUIDPreservedSymbols is empty. In such case we consider all symbols live. Having all symbols live doesn't however mean that some of the can't be readonly. That's why we see some additional internalizations happening.


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

https://reviews.llvm.org/D69561