This is an archive of the discontinued LLVM Phabricator instance.

Make aliases explicit in the module summary
ClosedPublic

Authored by mehdi_amini on Apr 6 2016, 11:20 AM.

Details

Summary

To be able to work accurately on the reference graph when taking decision
about internalizing, promoting, renaming, etc. We need to have the alias
information explicit.

This needs a bit of cleanup and a few more test, but you may have some early comments on the general way of representing this.

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to Make alias explicit in the module summary.
mehdi_amini updated this object.
mehdi_amini added a reviewer: tejohnson.
mehdi_amini set the repository for this revision to rL LLVM.
mehdi_amini added a subscriber: llvm-commits.
mehdi_amini updated this revision to Diff 52880.Apr 6 2016, 6:55 PM

Added a test

tejohnson added inline comments.Apr 6 2016, 10:05 PM
include/llvm/Bitcode/LLVMBitCodes.h
215

COMBINED_ALIAS

lib/Bitcode/Reader/BitcodeReader.cpp
5857

Note somewhere that it is expected that all alias records should follow all of the other summary records?

Also, I think this will be an issue for anonymous functions since won't get a summary for them (see comment below in bitcode writer)

lib/Bitcode/Writer/BitcodeWriter.cpp
2928

Comment is wrong now. With this change we will no longer get a summary emitted for anonymous functions.

2964

Do this renaming in a different NFC commit?

3120

The name seems off - this is an offset not an ID.

3132

continue is unnecessary

lib/Transforms/IPO/FunctionImport.cpp
90

Anonymous function may not have summary, see earlier comment

347

Lost this comment, I think it was useful.

test/Bitcode/thinlto-function-summary.ll
10

This is a case where aliasee is an anonymous function and doesn't have a summary. I would expect the reading side to hit some of the asserts I called out earlier.

mehdi_amini added inline comments.Apr 6 2016, 10:10 PM
lib/Bitcode/Writer/BitcodeWriter.cpp
2928

Indeed. Can we just "rename" anonymous functions in the compile-phase of the ThinLTO pipeline?
Then we would just guarantee to not have any anonymous function.

2964

Will do.

3120

Yes, I started with an ID before realizing I needed an offset, will rename.

lib/Transforms/IPO/FunctionImport.cpp
347

Indeed, will restore!

mehdi_amini marked 8 inline comments as done.

Address comments. Assuming a separate patch will introduce renaming for anonymous functions.

tejohnson added inline comments.Apr 7 2016, 9:25 AM
lib/Bitcode/Reader/BitcodeReader.cpp
5840

Maybe "Aliases must be emitted (and parsed) after all FS_PERMODULE entries, as they expect all aliasee summaries to be available." Similar for the COMBINED_ALIAS

lib/Bitcode/Writer/BitcodeWriter.cpp
2928

I guess? Don't know what expectations there are for anonymous functions, but that would certainly make life easier.

2932

Won't this cause assert for anonymous function in thinlto-function-summary.ll test?

mehdi_amini retitled this revision from Make alias explicit in the module summary to Make aliases explicit in the module summary.Apr 7 2016, 5:34 PM
mehdi_amini updated this revision to Diff 52988.Apr 7 2016, 5:50 PM
mehdi_amini marked 7 inline comments as done.

Address comment. Accept nameless functions but don't write an entry in the summary for alias to them.

tejohnson added inline comments.Apr 7 2016, 10:24 PM
lib/Bitcode/Reader/BitcodeReader.cpp
5925

s/FS_PERMODULE/FS_COMBINED/

lib/Bitcode/Writer/BitcodeWriter.cpp
2929

Saw that you sent a patch to rename them on which this is dependent (will review that tomorrow morn). Assuming that goes in first, perhaps this should stay an assert? Ditto for the continue you added below to the alias loop.

mehdi_amini added inline comments.Apr 8 2016, 1:29 AM
lib/Bitcode/Writer/BitcodeWriter.cpp
2929

That's why I put the assert originally. But then I wasn't sure anymore: should we forbid llvm-as -module-summary test.ll when it contains an alias to an anonymous function?
The current implementation will accept it, but the alias will just not be present in the summary.

2932

Yes, until we name them :)

mehdi_amini marked an inline comment as done.Apr 8 2016, 1:29 AM
tejohnson added inline comments.Apr 8 2016, 7:03 AM
lib/Bitcode/Writer/BitcodeWriter.cpp
2929

Looked at the anon function renaming pass - ok I see the issue is that it is a pass that it won't kick in for llvm-as. I think that should be fixed - similar to the way with D18763 I am computing the Index in llvm-as by invoking the builder directly, I think that the renaming in your new pass should be structured so that it is done in a separate class that can be invoked either from the pass or out of pass. I don't think we want to get a different index when there are anonymous functions that come directly from source through the pass manager vs from LLVM assembly via llvm-as.

And then this can be turned back into an assert to catch anything unexpected.

mehdi_amini added inline comments.Apr 8 2016, 7:34 AM
lib/Bitcode/Writer/BitcodeWriter.cpp
2929

I already made it a utility (there is a free function in a public header), because my original plan was to do exactly what you mention above. However I was not super comfortable making llvm-as depends on the Transformations library, or modifying the IR itself...

tejohnson added inline comments.Apr 8 2016, 7:45 AM
lib/Bitcode/Writer/BitcodeWriter.cpp
2929

Ah, see that now about the utility function.

I think it should be moved out of Transforms into some place more accessible.

IMO changing the IR in llvm-as for this purpose is acceptable, but not sure what the general philosophy is.

mehdi_amini added inline comments.Apr 8 2016, 9:03 AM
lib/Bitcode/Writer/BitcodeWriter.cpp
2929

I'll wait for Duncan's opinion on this (he's OOO right now).

We would break round-tripping through llvm-as | llvm-dis when summary is enabled.

tejohnson added inline comments.Apr 8 2016, 9:26 AM
lib/Bitcode/Writer/BitcodeWriter.cpp
2929

What does it mean to break round-tripping? Is it required that the output be identical to the input? Because once I add serialization of the index, that will be a difference.

mehdi_amini added inline comments.Apr 8 2016, 9:31 AM
lib/Bitcode/Writer/BitcodeWriter.cpp
2929

I just meant that the IR is changed here.
I'm not totally opposed to it, but I'd rather have someone else giving an opinion before doing it.

tejohnson added inline comments.Apr 8 2016, 12:18 PM
lib/Bitcode/Writer/BitcodeWriter.cpp
2943

I think this loop should be removed? Doesn't the new FS_ALIAS support apply to references as well?

mehdi_amini updated this revision to Diff 53087.Apr 8 2016, 2:06 PM

Remove obsolete loop pointed by Teresa

mehdi_amini marked 8 inline comments as done.

Error when a nameless function is encountered during summary writing, this is possible after D19032 (move from llvm-as to opt)

tejohnson added inline comments.Apr 13 2016, 7:56 AM
lib/Bitcode/Writer/BitcodeWriter.cpp
2929

Maybe "should have been renamed by anonymous function renaming pass"?

test/Bitcode/thinlto-function-summary.ll
1

Needs merge. But I think there is still an issue here, as opt will also need to be invoked with the -name-anon-functions to avoid the new assert. Which makes me wonder - should the -module-summary option to opt force enable -name-anon-functions? Or if not, at least warn if that wasn't specified?

Rebase, fix some tests that require the name-anon-function pass

Improve a little bit one test

Anything else here?

tejohnson accepted this revision.Apr 15 2016, 6:32 AM
tejohnson edited edge metadata.

Thanks! One unaddressed nit noted below, and a suggestion for future patch.

lib/Bitcode/Reader/BitcodeReader.cpp
5925

This one still needs a fix.

test/Bitcode/thinlto-function-summary.ll
1

Not for this patch, but I still think it would be useful to make opt either force enable -name-anon-functions when -module-summary is supplied, or at least warn if it wasn't.

This revision is now accepted and ready to land.Apr 15 2016, 6:32 AM
mehdi_amini closed this revision.Apr 16 2016, 12:02 AM
mehdi_amini marked 3 inline comments as done.

r266517