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.
Details
- Reviewers
power-llvm-team nemanjai lei hfinkel - Group Reviewers
Restricted Project - Commits
- rG75828ef615da: [PowerPC][Future] Initial support for PCRel addressing for constant pool loads
Diff Detail
Event Timeline
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?
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.
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.
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.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
2413 | The last comment and this comment could be made similar. I prefer this one. |
Just found one more typo.
llvm/lib/Target/PowerPC/PPCInstrPrefix.td | ||
---|---|---|
340 | s/temporatry/temporary |
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 | ||
2771 | Just return DAG.getNode(...); | |
llvm/lib/Target/PowerPC/PPCInstrInfo.td | ||
326 | 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 | 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. |
llvm/test/CodeGen/PowerPC/csr-split.ll | ||
---|---|---|
13–41 | 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. |
llvm/lib/Target/PowerPC/PPC.h | ||
---|---|---|
102 | "Fixup is VK_PCREL". or is it the variant kind ? | |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
2319 | 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. | |
2404 | Update the comment. |
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.
"Fixup is VK_PCREL". or is it the variant kind ?