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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson requested changes to this revision.
tejohnson added a comment.
This revision now requires changes to proceed.

Thanks for working on this! I had been thinking about adding this recently but didn't have a chance yet. I have a few suggestions below. One thing that should be considered for a follow-up enhancement is to consider the refs themselves for importing (similar to the way we follow chains of calls when doing function importing).



================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:283
+static bool hasRefsPreventingImport(const GlobalVarSummary *GVS) {
+  return !GVS->maybeReadOnly() && GVS->refs().size();
+};
----------------
The name of the maybeReadOnly is a little confusing here, although I know we expect that analysis to have finalized this flag by now. Can you add a flag to the summary index to indicate that the attribute propagation has completed (similar to the WithGlobalValueDeadStripping flag that indicates liveness analysis is complete), and then add a helper on the summary to return the maybeReadOnly value that asserts the flag is set, and change the uses sites to use that instead? E.g. something like a readOnly() method, and probably writeOnly() one for completeness.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:318
           canImportGlobalVar(RefSummary.get()) &&
+          !hasRefsPreventingImport(cast<GlobalVarSummary>(RefSummary.get())) &&
           !LocalNotInModule(RefSummary.get())) {
----------------
I think it would be better to call this from within canImportGlobalVar to keep the correctness checks centralized.


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

https://reviews.llvm.org/D69561