[PATCH] D63444: [ThinLTO] Optimize write-only globals out

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks! A bunch of misc comments/suggestions below.



================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:743
+    // Unlike reference (ValueInfo), GlobalVarSummary has both
+    // ReadOnly and WriteOnly bits set in permodule summaries.
+    // This happens, because attribute propagation occurs on
----------------
This is a little confusing. Maybe change the names to "PossibleReadOnly" and "PossibleWriteOnly" or something like that?


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:285
+        NonVolatileStores.push_back(&I);
+        // Add all reference edges from first operand of store.
+        // We can't tell if such refernces are read or write only.
----------------
Clarify that this first operand is the stored value. Also clarify why can't we just let these get added naturally when we do the deferred invocations of findRefEdges - I guess it is because we want to capture any references to pointers being considered (e.g. that they don't escape)?


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:286
+        // Add all reference edges from first operand of store.
+        // We can't tell if such refernces are read or write only.
+        Value *Stored = I.getOperand(0);
----------------
s/refernces/references/. Also, the references you are referring to on this line is (I think) different than the references you mention on the prior line - can you adjust the comment to reflect (or maybe I am confused??).


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:292
+          RefEdges.insert(Index.getOrInsertValueInfo(GV));
+        else if (auto *U = dyn_cast<User>(Stored))
+          findRefEdges(Index, U, RefEdges, Visited);
----------------
is there a fall through else case that we aren't handling?


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:419
   // Regular LTO module doesn't participate in ThinLTO import,
   // so no reference from it can be readonly, since this would
   // require importing variable as local copy
----------------
adjust comment about writeonly as well. Actually, would it be better to simply skip the special case handling of non-volatile loads and stores in the above loop if !IsThinLTO in the first place? Then skip all of the handling for those here as well.


================
Comment at: lib/AsmParser/LLParser.cpp:7768
+  if (WriteOnly)
+    Fwd->setReadOnly();
 }
----------------
should be calling setWriteOnly. Can you add a test that would have caught this?


================
Comment at: lib/AsmParser/LLParser.cpp:8605
+  GVarFlags.WriteOnly = false;
+  if (Lex.getKind() == lltok::comma) {
+    Lex.Lex();
----------------
This code won't handle the case where a GVar is writeonly but not readonly. Suggest turning into a:
  do {
...
  } while (EatIfPresent(lltok::comma));

loop (there are some examples in other summary parsing functions). As a bonus, that allows specifying the flags in either order.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5381
+                           unsigned WOCnt) {
   // Read-only refs are in the end of the refs list.
+  assert(ROCnt + WOCnt <= Refs.size());
----------------
Update comment


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5587
       unsigned RefArrayStart = 2;
-      GlobalVarSummary::GVarFlags GVF;
+      GlobalVarSummary::GVarFlags GVF(false, false);
       auto Flags = getDecodedGVSummaryFlags(RawFlags, Version);
----------------
Can you add comments for the const parameters? E.g. /*ReadOnly=*/


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5699
       unsigned RefArrayStart = 3;
-      GlobalVarSummary::GVarFlags GVF;
+      GlobalVarSummary::GVarFlags GVF(false, false);
       auto Flags = getDecodedGVSummaryFlags(RawFlags, Version);
----------------
Ditto on const params


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:145
 //   b. referenced by any global variable initializer
 //   c. referenced by a function and reference is not readonly
 //
----------------
this last one doesn't go with the writeonly case, probably needs updating


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:147
 //
+//   Also we clear writeonly attribute is reference to a variable is readonly
+//   and readonly attribute if reference is writeonly. This means that having
----------------
s/is reference/if reference/


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:148
+//   Also we clear writeonly attribute is reference to a variable is readonly
+//   and readonly attribute if reference is writeonly. This means that having
+//   two different functions of which one is reading variable 'foo' and other
----------------
"and clear readonly attribute"... sounds clearer to mean


================
Comment at: lib/IR/ModuleSummaryIndex.cpp:162
 
       // Global variable can't be marked read only if it is not eligible
       // to import since we need to ensure that all external references
----------------
comment needs update for write only case in a few places


================
Comment at: lib/LTO/LTO.cpp:196
+    if (auto *GVS = dyn_cast<GlobalVarSummary>(GS)) {
+      AddUnsigned(GVS->isReadOnly() | GVS->isWriteOnly());
+    }
----------------
Probably they should each be added in individually or we'd end up with the same hash if in one version summary was read only and in another it is write only.


================
Comment at: test/ThinLTO/X86/index-const-prop.ll:16
 
+; Check that we optimize write-only variables
+; RUN: llvm-lto -thinlto-action=import -exported-symbol=main2  %t1.bc -thinlto-index=%t3.index.bc -o %t1.imported.bc -stats 2>&1 | FileCheck %s --check-prefix=STATS2
----------------
Clearer to pull these new test cases into different files, since they aren't about constant prop


================
Comment at: test/ThinLTO/X86/index-const-prop2.ll:54
+; RUN: llvm-dis %t5.1.5.precodegen.bc -o - | FileCheck %s --check-prefix=CODEGEN2
+; Check that gFoo and gBar were eliminated from source module together
+; with corresponsing stores
----------------
I see the check below that the stores were eliminated from the source module, but not where we are checking that the variables were eliminated altogether?


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

https://reviews.llvm.org/D63444