This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][Future] Add initial support for PC Relative addressing for global values that require GOT indirect addressing
ClosedPublic

Authored by stefanp on Mar 12 2020, 6:50 AM.

Details

Summary

This patch adds PCRelative support for global addresses that may not be known at link time and may require access through the GOT.

Diff Detail

Event Timeline

stefanp created this revision.Mar 12 2020, 6:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2020, 6:50 AM
nemanjai accepted this revision.Mar 17 2020, 7:21 AM

Approving this even though there is a problem with the logic in SelectAddressPCRel() as I trust that you will fix the obvious bug when committing this.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
2597–2598

Won't this always evaluate to true if ConstPoolNode is not null? Perhaps this should have been bitwise-and?
Similarly below.

3054

Similarly to the previous patch, this needs to be removed from the AIX block.

This revision is now accepted and ready to land.Mar 17 2020, 7:21 AM
anil9 added a subscriber: anil9.Mar 25 2020, 10:18 PM
anil9 added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
2597–2598

This one and the above conditions, I guess you mistakenly did bitwise OR instead of AND.
And better to put the second line in brackets, may be the precedence of the operations are correct, but given we are doing two types of operations ( logical and bitwise ) having brackets will make it more readable without having to worry about operator precedence.

This revision was automatically updated to reflect the committed changes.
kamaub added a subscriber: kamaub.Apr 17 2020, 10:29 AM

Sorry I missed that git clang-format changed whitespace on unrelated lines.
Pulling out simply because of this seems like unnecessary churn for the bots
especially since the formatting is an improvement.
If someone would like me to pull it, I certainly don't mind.
I will definitely be more vigilant next time.

I commented on the commit b771c4a8, FYI