This is an archive of the discontinued LLVM Phabricator instance.

[LTO] Asm undefs should be included in llvm.compiler_used
ClosedPublic

Authored by davide on Jun 21 2016, 5:20 PM.

Diff Detail

Event Timeline

davide updated this revision to Diff 61467.Jun 21 2016, 5:20 PM
davide retitled this revision from to [LTO] Asm undefs should be included in llvm.compiler_used.
davide updated this object.
davide added reviewers: rafael, pcc.
davide added a subscriber: llvm-commits.
ruiu added a subscriber: ruiu.Jun 21 2016, 6:31 PM
ruiu added inline comments.
ELF/LTO.cpp
209–220

This function is already too long. Can you separate this code as a new function?

220

AsmUndefinedRefs.insert(Name) since it is a set.

davide updated this revision to Diff 61504.Jun 21 2016, 10:13 PM

Rui's comments.

ruiu added inline comments.Jun 21 2016, 10:36 PM
ELF/LTO.cpp
158

remove llvm::.

169

I think you don't need this brace to create a scope. raw_svector_ostream is unbuffered so Buffer is always up-to-date.

173

Why not Buffer.str()? I'd rename Buffer Name and just call AsmUndefinedRefs.insert(Name.str())

davide updated this revision to Diff 61506.Jun 21 2016, 10:56 PM

Rui's comments (2).

davide updated this revision to Diff 61507.Jun 21 2016, 10:57 PM
ruiu added a comment.Jun 21 2016, 11:05 PM

Looks good, but I guess you want someone else to work on LTO to sign off.

rafael accepted this revision.Jun 22 2016, 7:40 AM
rafael edited edge metadata.

LGTM with one last pass of reviews.

ELF/LTO.cpp
28

Please make sure this builds with -DBUILD_SHARED_LIBS=ON.

159

s/no asm/not an assembly symbol/ I guess?

163

While are the comment lines so short?

I think you can drop the "referenced": This is an undefined reference to a symbol in asm".

test/ELF/lto/asmundef.ll
23

Check that foo is internalized.

This revision is now accepted and ready to land.Jun 22 2016, 7:40 AM
This revision was automatically updated to reflect the committed changes.