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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:210
+  void setReadOnly() {
+    // We expect ro/wo attribute to set only once during
+    // ValueInfo lifetime.
----------------
tejohnson wrote:
> Is this checked anywhere?  Should the assert be checking that the bit is not yet set?
As noted in my last comments - we aren't validating this assumption anywhere. Should this check that getAccessSpecifier is 0 before we set? Ditto below for setWriteOnly.


================
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.
----------------
evgeny777 wrote:
> tejohnson wrote:
> > 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)?
> The idea of deferred processing is that at some moment we know that all further added references are either readonly or writeonly, so we can set correct bits in ValueInfo. We can't say anything about objects referenced by first operand of store, for instance this instruction:
> 
> ```
> store i32* @value, i32** @value_ptr
> ```
> is identical to C construct:
> ```
> int *value_ptr = &value;
> ```
> And while value_ptr can be considered write-only we can't say anything about value
Ok sure, just clarify that "second operand" means the pointer address being stored, and "first operand" means the value we are storing.


================
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
----------------
tejohnson wrote:
> 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.
I see you moved the IsThinLTO check up as suggested, but can we also skip this whole block of code (that handles NonVolatileLoads/Stores and then processes the resulting Load/StoreRefEdges sets) if !IsThinLTO? I realize this code won't do anything since those sets will be empty, but I think it would be clearer.


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:290
+          // All references from second operand of store can be considered
+          // write-only if we haven't added them previously to RefEdges with
+          // when processing some other instruction. References from first
----------------
Remove "with" at end of above line? I.e. I think it should be "to RefEdges when processing"


================
Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:244
     auto *GVS = SL.empty() ? nullptr : dyn_cast<GlobalVarSummary>(SL[0].get());
-    if (GVS && GVS->isReadOnly())
+    // At this stage "maybe" is "definitly"
+    if (GVS && (GVS->maybeReadOnly() || GVS->maybeWriteOnly()))
----------------
s/definitly/definitely/


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

https://reviews.llvm.org/D63444