This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][AIX] Add toc-data support for 64-bit AIX
ClosedPublic

Authored by yoalione on Nov 26 2021, 11:56 AM.

Details

Summary

The patch handles the addition of global variables to the table of contents for 64-bit AIX. This was previously supported for 32-bit (https://reviews.llvm.org/D101178).

The patch expands toc-data attribute support to 64-bit.

Diff Detail

Event Timeline

yoalione created this revision.Nov 26 2021, 11:56 AM
yoalione requested review of this revision.Nov 26 2021, 11:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2021, 11:56 AM
yoalione added a reviewer: Restricted Project.Nov 26 2021, 11:58 AM

Nice work Yousuf. Can you start by adding the context for the patch (for example if you used git diff or git show then adding -U9999 will give you 9999 lines of surrounding context in the diff), and running clang format on the patch?

yoalione updated this revision to Diff 390117.EditedNov 26 2021, 1:38 PM

Address comments and combine common code.

sfertile added inline comments.Nov 29 2021, 8:29 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
892

minor nit: no need to have an if/else for this. We don't have to be too formal with assert messages and having "Invalid operand for ADDItoc[8]." is fine.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5833

Minor nir: I'm guessing this changed because you ran clang-format on the whole file? We try not to touch otherwise unchanged lines. Can you put it back to what it originally was.

yoalione updated this revision to Diff 390399.Nov 29 2021, 10:09 AM

Address review comments.

yoalione marked 2 inline comments as done.Nov 29 2021, 10:16 AM
This revision is now accepted and ready to land.Nov 29 2021, 12:49 PM

Nice work Yousuf. Can you start by adding the context for the patch (for example if you used git diff or git show then adding -U9999 will give you 9999 lines of surrounding context in the diff), and running clang format on the patch?

Please note that you probably need at least another 9 to make it -U99999 since there will be files that are well over 10kloc (looking at you PPCISelLowering.cpp). :)

My comments are just minor nits that don't require another review and can be addressed when committing the patch.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5845

The isAIXABI check here should be unnecessary since we would break out of here at line 5795. I think it would be clearer if we just add an assert for isAIXABI just above this line instead.

llvm/lib/Target/PowerPC/PPCInstr64Bit.td
776

This is already in a isCodeGenOnly = 1 block isn't it?

yoalione updated this revision to Diff 390728.Nov 30 2021, 8:07 AM

Address review comments.

yoalione marked 2 inline comments as done.Nov 30 2021, 8:11 AM
yoalione added inline comments.
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5845

I think it should be within the if (so a line after).