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

[Eugene Leviant via Phabricator via llvm-commits]

evgeny777 added inline comments.


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


================
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);
----------------
tejohnson wrote:
> is there a fall through else case that we aren't handling?
findRefEdges can't be called with anything except llvm::User object. This means we can't call it wth, say, integer constant.


================
Comment at: lib/AsmParser/LLParser.cpp:7768
+  if (WriteOnly)
+    Fwd->setReadOnly();
 }
----------------
tejohnson wrote:
> should be calling setWriteOnly. Can you add a test that would have caught this?
Nice catch. I've modified tests/Assembler/thinlto-summary.ll to address this


================
Comment at: lib/AsmParser/LLParser.cpp:8605
+  GVarFlags.WriteOnly = false;
+  if (Lex.getKind() == lltok::comma) {
+    Lex.Lex();
----------------
tejohnson wrote:
> 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.
Makes sense. See thinlto-summary.ll for test case


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

https://reviews.llvm.org/D63444