This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Skip BlockAddresses while replacing uses in function import
ClosedPublic

Authored by dmikulin on Feb 7 2018, 10:02 AM.

Diff Detail

Event Timeline

dmikulin created this revision.Feb 7 2018, 10:02 AM

I'd like @pcc to comment. I noticed there are a number of other places in this file where we will still be calling replaceAllUsesWith on a Function. How do we know which should use which method?

pcc added a comment.Feb 7 2018, 12:17 PM

I'd like @pcc to comment. I noticed there are a number of other places in this file where we will still be calling replaceAllUsesWith on a Function. How do we know which should use which method?

We need to use replaceUsesExceptBlockAddr if the function that we are replacing is a function definition. As far as I can tell, in all other cases we are replacing function declarations.

llvm/test/ThinLTO/X86/blockaddr-import.ll
2

Can this be written as an IR-level test of the pass? See for example llvm/test/Transforms/LowerTypeTests/import-icall.ll.

dmikulin updated this revision to Diff 133464.Feb 8 2018, 11:40 AM

Re-wrote the test.

pcc added inline comments.Feb 8 2018, 12:01 PM
llvm/test/Transforms/LowerTypeTests/blockaddr-import.ll
1 ↗(On Diff #133464)

I don't think you need -O2 here.

29 ↗(On Diff #133464)

Please use FileCheck to check that the blockaddress refers to the correct function.

dmikulin marked an inline comment as done.Feb 8 2018, 12:52 PM
dmikulin added inline comments.
llvm/test/Transforms/LowerTypeTests/blockaddr-import.ll
1 ↗(On Diff #133464)

I do not need the whole O2 pipeline, but the verifier relies on some analysis passes to set address-taken flags to assert. I changed it to -O1.

29 ↗(On Diff #133464)

Added a check to make sure blockaddress refers to the new function, m.cfi.

dmikulin marked 2 inline comments as done.Feb 8 2018, 1:07 PM
dmikulin added inline comments.
llvm/test/Transforms/LowerTypeTests/blockaddr-import.ll
1 ↗(On Diff #133464)

Looks like having the check to make sure the new function is used in blockaddress might be sufficient. I'll take out the -O option.

dmikulin updated this revision to Diff 133478.Feb 8 2018, 1:08 PM
dmikulin marked an inline comment as done.
dmikulin marked an inline comment as done.

Fixed the test.

pcc accepted this revision.Feb 8 2018, 1:33 PM

LGTM

llvm/test/Transforms/LowerTypeTests/Inputs/blockaddr-import.yaml
2–6 ↗(On Diff #133478)

I think you can remove this part.

llvm/test/Transforms/LowerTypeTests/blockaddr-import.ll
11 ↗(On Diff #133478)

Is this function still necessary?

This revision is now accepted and ready to land.Feb 8 2018, 1:33 PM
dmikulin marked 2 inline comments as done.Feb 8 2018, 1:53 PM
This revision was automatically updated to reflect the committed changes.
pcc added inline comments.Feb 8 2018, 2:22 PM
llvm/test/Transforms/LowerTypeTests/Inputs/blockaddr-import.yaml
2–6 ↗(On Diff #133478)

It looks like this wasn't addressed.