[PATCH] D30738: Don't internalize llvm GV's with InternalizeLinkedSymbols

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: clang/include/clang/CodeGen/CodeGenAction.h:40
+    /// If true, we use LLVM module internalizer.
+    bool Internalize;
+
----------------
Something I completely missed while reviewing D30792 - I don't see how this is ever getting set. It looks like this line would need to be modified:

(From http://llvm-cs.pcc.me.uk/tools/clang/lib/CodeGen/CodeGenAction.cpp#819)

      LinkModules.push_back(
          {std::move(ModuleOrErr.get()), F.PropagateAttrs, F.LinkFlags});

That's where the values from BitcodeFileToLink, which has the new Internalize flag being set CompilerInvocation.cpp, should be used to initialize the new LinkModule object. It looks like there are tests in tools/clang/CodeGenCUDA/ that are checking to see if internalization happened properly and should fail.

Ah - because of the field ordering, I think the LinkFlags value is essentially being applied to the Internalize flag, so it may be getting lucky on that front. But then I would think the LinkFlags field would be uninitialized, and the desired flag of llvm::Linker::Flags::LinkOnlyNeeded not being set in the CUDA case. Can you look at why test/CodeGenCUDA/link-device-bitcode.cu, which appears to test for both internalization and the only needed linking, isn't failing? If the test is not sufficient, please augment  it so that it is failing due to this problem.


================
Comment at: llvm/test/Linker/link-flags.ll:25
+DI-LABEL: define internal i32 @foo.6()
+DI-LABEL: define i32 @foo()
----------------
Suggest making the type of the new @foo different, so it is clear which one is internalized vs not.


Repository:
  rL LLVM

https://reviews.llvm.org/D30738