Page MenuHomePhabricator

[PowerPC][Future] Add initial support for PC Relative addressing for constant pool loads
ClosedPublic

Authored by stefanp on Feb 12 2020, 7:00 AM.

Details

Summary

Add initial support for PC Relative addressing for constant pool loads.
This includes adding a new relocation for @pcrel and adding a new PowerPC flag to identify PC relative addressing.

Diff Detail

Event Timeline

stefanp created this revision.Feb 12 2020, 7:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2020, 7:00 AM
jsji added a comment.Feb 12 2020, 7:11 AM

Note that Hexagon had already defined VK_Hexagon_PCREL so I modified the Hexagon code to use a non target specific VK_PCREL. The change with respect to Hexagon should be NFC.

Can we split it to be a NFC patch and reviewed by Hexagon guys?

NeHuang added inline comments.
llvm/test/CodeGen/PowerPC/constant-pool.ll
44

exceeding 80 columns

52

exceeding 80 columns

84

exceeding 80 columns

nemanjai added inline comments.Feb 14 2020, 9:39 AM
llvm/test/CodeGen/PowerPC/constant-pool.ll
44

Generally speaking, we don't try to keep test cases to 80 columns.

Created a new review for only the NFC Hexagon changes here: https://reviews.llvm.org/D74788
Next, I will rework this patch.

stefanp updated this revision to Diff 245713.Feb 20 2020, 12:12 PM

Cleaned up patch and removed the NFC part.
Also added a couple of new patterns to the td file and added a new PPCISD node MAT_PCREL_ADDR. The only purpose of this new node is to materialize PC Relative addresses.

stefanp updated this revision to Diff 246855.Feb 26 2020, 4:37 PM

Removed the PCRelative guard that was incorrectly placed in PPCTargetLowering::SelectAddressRegRegOnly.
Removed the isPrefix class from the td file as it was not required.
Added more comments to the PPCInstrPrefix.td.

stefanp edited the summary of this revision. (Show Details)Feb 26 2020, 4:38 PM
stefanp updated this revision to Diff 246863.Feb 26 2020, 6:11 PM

Missed a tconstpool that should have been a pcreladdr.

amyk added a subscriber: amyk.Mar 1 2020, 2:00 PM
amyk added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
2573

I think it would be good if we had doxygen /// comments for this.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
375

s/meterialize/materialize

anil9 added a subscriber: anil9.Mar 3 2020, 6:02 AM
anil9 added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
2445

The last comment and this comment could be made similar. I prefer this one.

stefanp updated this revision to Diff 249206.Mar 9 2020, 1:52 PM

Fixed comments in the code as requested by reviewers.

stefanp updated this revision to Diff 249598.Mar 11 2020, 6:02 AM

Accidentally added unnecessary code when I did a rebase. Removed that now.

amyk added a comment.Mar 14 2020, 3:12 PM

Just found one more typo.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
340

s/temporatry/temporary

nemanjai accepted this revision.Mar 17 2020, 3:49 AM

LGTM aside from some minor nits. Feel free to address those on the commit.

llvm/lib/Target/PowerPC/PPC.h
103

Please add to the comment the reasoning behind this being 16 rather than the next power of 2.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
2800

Just return DAG.getNode(...);

llvm/lib/Target/PowerPC/PPCInstrInfo.td
323

All the other PPC-specific nodes start with PPC. Please maintain this convention.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td
347

This sounds like wishful thinking and not like a commitment to provide this refactoring in the near future. Please change it to:

// Once pc-relative implementation is complete, a set of follow-up patches will
// address this refactoring and the AddedComplexity will be removed.
351

Nit: throughout this block, the name of the pc-relative address seems to suggest that this is only for constant pool entries (i.e. cp). However, this is presumably useful for any pc-relative memory operations. Perhaps $addr or $src.

355

Nit, the alignment is wrong here. When splitting on two lines, the input and output patterns should be aligned in the same column.

llvm/test/CodeGen/PowerPC/csr-split.ll
3 ↗(On Diff #249598)

What is the addition to this test meant to accomplish? I do not see any constant pool loads in the test - or anything related to this patch TBH.

This revision is now accepted and ready to land.Mar 17 2020, 3:49 AM
NeHuang added inline comments.Mar 17 2020, 9:10 AM
llvm/test/CodeGen/PowerPC/csr-split.ll
13 ↗(On Diff #249598)

No PCRel instructions added for these test cases when add "mcpu=future". There are some non-GOT indirect-access load patterns but not constant pool loads in the test. We should revert the changes in this file. (most probably this file was touched by the old design.

anil9 added inline comments.Apr 2 2020, 1:55 PM
llvm/lib/Target/PowerPC/PPC.h
102

"Fixup is VK_PCREL". or is it the variant kind ?

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
2351

In the comment above the function it says when it returns false, now that you have another condition when it returns false I think you should update the comment.

2436

Update the comment.

Looks like this breaks tests on Windows: http://45.33.8.238/win/12548/step_11.txt

kamaub added a subscriber: kamaub.EditedApr 9 2020, 11:16 AM

Looks like this breaks tests on Windows: http://45.33.8.238/win/12548/step_11.txt

Thank you, we are looking into the problem right now, it will likely be a simple fix so we will push(commit a fix) rather than revert.

This revision was automatically updated to reflect the committed changes.

Thanks for the fix!