This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Assembly representation of ReadOnly attribute
ClosedPublic

Authored by evgeny777 on Nov 20 2018, 6:42 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

evgeny777 created this revision.Nov 20 2018, 6:42 AM
tejohnson added inline comments.Nov 21 2018, 4:34 PM
lib/AsmParser/LLParser.cpp
7473 ↗(On Diff #174766)

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

evgeny777 added inline comments.Nov 22 2018, 1:16 AM
lib/AsmParser/LLParser.cpp
7473 ↗(On Diff #174766)

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.

tejohnson accepted this revision.Nov 22 2018, 8:08 AM

lgtm

lib/AsmParser/LLParser.cpp
7473 ↗(On Diff #174766)

Right, I forgot about the fact that the read only flag is set before we resolve the forward ref. Nevermind then.

This revision is now accepted and ready to land.Nov 22 2018, 8:08 AM
This revision was automatically updated to reflect the committed changes.