Page MenuHomePhabricator

[XCOFF][AIX] Handle llvm.used and llvm.compiler.used global array
ClosedPublic

Authored by jasonliu on Jul 22 2020, 2:24 PM.

Details

Summary

For now, just return and do nothing when we see llvm.used and llvm.compiler.used global array.
Hopefully, we could come up with a good solution later to prevent linker from eliminating symbols in llvm.used array.

Diff Detail

Event Timeline

jasonliu created this revision.Jul 22 2020, 2:24 PM
daltenty accepted this revision.Jul 23 2020, 9:15 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jul 23 2020, 9:15 AM
DiggerLin accepted this revision.Jul 23 2020, 1:02 PM
DiggerLin added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-used.ll
21

nit:
do we need to
; CHECK-NOT: llvm.used
and
; CHECK-NOT: @llvm.compiler.used
?

jasonliu updated this revision to Diff 280245.Jul 23 2020, 1:33 PM

Modified test case and added GV->hasAppendingLinkage() to avoid string comparison in most cases.

jasonliu marked an inline comment as done.Jul 23 2020, 1:34 PM
DiggerLin added inline comments.Jul 23 2020, 1:57 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1682

not sure llvm.global_ctors must have an appending Linkage or not, I can not any information that talk about the llvm.global_ctors must having an appending Linkage.
in the
https://llvm.org/docs/LangRef.html , I can find the description as "
The @llvm.used global is an array which has appending linkage. "

and
"appending
“appending” linkage may only be applied to global variables of pointer to array type. When two global variables with appending linkage are linked together, the two global arrays are appended together. This is the LLVM, typesafe, equivalent of having the system linker append together “sections” with identical names when .o files are linked"

but not for the llvm.global_ctors

jasonliu marked an inline comment as done.Jul 23 2020, 2:27 PM
jasonliu added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1682

I did a search for llvm.global_ctors and llvm.global_dtors in clang's unit test, and found all the occurrence of those are with appending linkage. And in my rough look up in the code, I don't find anything that could generate those variable with another linkage. So I think it's safe to assume these special global variables all have appending linkage. I don't really see a reason for those special global variables not to have an appending linkage. If there is a potential reason for them to not have that linkage, I could remove this quick escape path.

DiggerLin added inline comments.Jul 24 2020, 6:19 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1682

if you want to make the code more efficient, the four strings has a common part "llvm." can we compare whether start with "llvm." , if not return false first, otherwise compare one by one?

jasonliu marked an inline comment as done.Jul 24 2020, 7:08 AM
jasonliu added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1682

We are still doing string comparison effectively, and you are not saving much by comparing the common part (unless we have more cases here, right now we have only three cases). And then you need to do all extra substr to compare the remaining character. That's not very appealing to me.

jasonliu updated this revision to Diff 280462.Jul 24 2020, 7:46 AM

Added an assert to make sure we handled all "llvm." cases.

jasonliu marked an inline comment as done.Jul 24 2020, 7:49 AM
jasonliu added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1682

I added an assert below to make sure we know if there is something start with llvm. and we didn't handled it yet. That should address your concern.
I think anything starts with "llvm." would need some special handling, so the assert could help us capture future new llvm. global variable, and existing llvm. that we don't know yet.

DiggerLin accepted this revision.Jul 24 2020, 12:35 PM
DiggerLin marked an inline comment as done.

LGTM.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1682

thanks

This revision was automatically updated to reflect the committed changes.