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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5853
+          (Flags & 0x20) ||
+          (Version >= 5 && TheIndex.withGlobalValueDeadStripping()));
       break;
----------------
I don't follow the second check, since the WithGlobalValueDeadStripping flag was added to the index before attribute propagation was supported.


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:201
+    // permit import.
+    return WithAttributePropagation && !isReadOnly(GVS) && GVS->refs().size();
+  };
----------------
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.


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:205
+
+  // We don't import GV with references, because it can result
+  // in promotion of local variables in the source module.
----------------
Update comment


================
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
----------------
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).


================
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]]
----------------
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.


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

https://reviews.llvm.org/D69561