This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Optimize write-only globals out
ClosedPublic

Authored by evgeny777 on Jun 17 2019, 10:22 AM.

Details

Summary

This patch uses approach introduced in D49362 to internalize variables with write-only access. Optimizer in the backend eliminates them together with corresponding stores.

Here is test result using LLVM test suite:

Diff Detail

Event Timeline

evgeny777 created this revision.Jun 17 2019, 10:22 AM

Sounds like an interesting optimization. Is the global storage (from the other module) also eliminated in this case? If so, can you add a testcase for that?

include/llvm/IR/ModuleSummaryIndex.h
165–166

Would it be cleaner if you create a new enum bitfield for access specifier?

enum Access { ReadWrite = 0, ReadOnly = 1, WriteOnly =2, Invalid =3 };

This should make all the method below a bit cleaner.

evgeny777 marked an inline comment as done.Jun 18 2019, 12:17 AM

Is the global storage (from the other module) also eliminated in this case? If so, can you add a testcase for that?

Yes, optimized variable is internalized in both source and destination module. I'll add test case for that

include/llvm/IR/ModuleSummaryIndex.h
165–166

I'm not sure if this would make it cleaner. There is also a HaveGV flag in PointerIntPair, so to manipulate access bits as suggested you'll have to do something like this:

bool isValidAccessSpecifier() const {
    return (RefAndFlags.getInt() &  (Invalid << 1)) !=  (Invalid << 1);
}
void setWriteOnly() {
    assert(isValidAccessSpecifier());
    RefAndFlags.setInt(RefAndFlags.getInt() | (WriteOnly << 1));
 }

so technically almost the same as before

Added test case for internalization in the source module (see index-const-prop2.ll)

steven_wu accepted this revision.Jun 18 2019, 8:29 AM

LGTM.

include/llvm/IR/ModuleSummaryIndex.h
165–166

My suggestion was something like (simplified):

Access getAccess() const { return RefAndFlags.getInt() >> 1; }
void setAccess(Access status) { RefAndFlags.setInt(status <<1) }
bool isReadOnly() const { getAccess() == ReadOnly; }

This is has slightly different semantics than the one you have (it overwrites status and cannot get into invalid state). I am fine with current implementation as well.

This revision is now accepted and ready to land.Jun 18 2019, 8:29 AM

@tejohnson Teresa, do you have any comments/objections?

ormris added a subscriber: ormris.Jun 19 2019, 10:47 AM

I didn't get very far today, will finish the review tomorrow morning.

include/llvm/IR/ModuleSummaryIndex.h
210

Is this checked anywhere? Should the assert be checking that the bit is not yet set?

212

Probably better to check this after it is set below, to catch case of both being set more immediately.

216

ditto

745

s/think/thin/

Thanks! A bunch of misc comments/suggestions below.

include/llvm/IR/ModuleSummaryIndex.h
743

This is a little confusing. Maybe change the names to "PossibleReadOnly" and "PossibleWriteOnly" or something like that?

lib/Analysis/ModuleSummaryAnalysis.cpp
280

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

281

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

287

is there a fall through else case that we aren't handling?

426–427

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.

lib/AsmParser/LLParser.cpp
7779

should be calling setWriteOnly. Can you add a test that would have caught this?

8618

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.

lib/Bitcode/Reader/BitcodeReader.cpp
5380–5381

Update comment

5589–5590

Can you add comments for the const parameters? E.g. /*ReadOnly=*/

5702–5703

Ditto on const params

lib/IR/ModuleSummaryIndex.cpp
151–152

this last one doesn't go with the writeonly case, probably needs updating

154

s/is reference/if reference/

155

"and clear readonly attribute"... sounds clearer to mean

163–164

comment needs update for write only case in a few places

lib/LTO/LTO.cpp
196

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.

test/ThinLTO/X86/index-const-prop.ll
16 ↗(On Diff #205262)

Clearer to pull these new test cases into different files, since they aren't about constant prop

test/ThinLTO/X86/index-const-prop2.ll
54

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?

evgeny777 updated this revision to Diff 206118.Jun 22 2019, 7:04 AM
evgeny777 marked 18 inline comments as done.

Addressed review comments

lib/Analysis/ModuleSummaryAnalysis.cpp
280

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

287

findRefEdges can't be called with anything except llvm::User object. This means we can't call it wth, say, integer constant.

lib/AsmParser/LLParser.cpp
7779

Nice catch. I've modified tests/Assembler/thinlto-summary.ll to address this

8618

Makes sense. See thinlto-summary.ll for test case

tejohnson added inline comments.Jun 28 2019, 12:12 PM
include/llvm/IR/ModuleSummaryIndex.h
210

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.

lib/Analysis/ModuleSummaryAnalysis.cpp
280

Ok sure, just clarify that "second operand" means the pointer address being stored, and "first operand" means the value we are storing.

290

Remove "with" at end of above line? I.e. I think it should be "to RefEdges when processing"

426–427

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.

lib/Transforms/Utils/FunctionImportUtils.cpp
244

s/definitly/definitely/

evgeny777 updated this revision to Diff 207551.Jul 2 2019, 6:45 AM

Addressed

tejohnson accepted this revision.Jul 2 2019, 8:12 AM

LGTM with one minor comment nit below. Thanks!

lib/Analysis/ModuleSummaryAnalysis.cpp
292

Avoid double negative by changing to "can't be treated either as read- or as write-only..."

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 7:15 AM
rnk added a subscriber: rnk.Jul 3 2019, 5:06 PM

I suspect this caused https://crbug.com/981168, so I reverted this in rL365097. The steps to repro are to build LLD with thinlto and run check-lld on Linux. Some of the wasm tests fail because the WasmSym::MemoryBase global variable is wrongly internalized. I'll try to confirm that this is the culprit and provide more concrete repro steps momentarily, but I want to get things back to green before the US holidays.

rnk added a comment.Jul 3 2019, 5:50 PM

Just wanted to confirm, after reverting this, I was able to build and test LLD with ThinLTO enabled again, so this did cause the issue.

Unfortunately, I have not been able to come up with a reduced test case for the issue, so I will have to leave the instructions at "run check-lld with thinlto enabled".