[PATCH] D54754: [ThinLTO] Assembly representation of ReadOnly attribute

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: lib/AsmParser/LLParser.cpp:7473
 
-static ValueInfo EmptyVI =
-    ValueInfo(false, (GlobalValueSummaryMapTy::value_type *)-8);
+auto FwdVIRef = (GlobalValueSummaryMapTy::value_type *)-8;
+
----------------
evgeny777 wrote:
> tejohnson wrote:
> > Is this change a necessity, or just a cleanup? If the latter, would be better to split into a separate NFC patch. I like the name change from EmptyVI to the more accurate/descriptive FwdVIRef.
> > 
> > But is there any advantage in having it just be the ref and not the whole VI (i.e. a FwdVI)? It requires more code to do the comparison (getRef() invocations).
> Since now some of forward references are read-only and some are not. This means we can't check for forward declaration simply comparing ```VI == EmptyVI```, can we? As far as I understand there is no overloaded operator '== ' in ValueInfo and this means we'll do member wise comparison and in such case read only ValueInfo will not be equal to writable even if pointer value is the same.
Right, I forgot about the fact that the read only flag is set before we resolve the forward ref. Nevermind then.


https://reviews.llvm.org/D54754