Page MenuHomePhabricator

[PPC][GlobalISel] Add initial GlobalIsel infrastructure
ClosedPublic

Authored by kbarton on Jul 2 2020, 3:28 PM.

Details

Summary

This adds the initial GlobalISel skeleton for PowerPC. It can only run
ir-translator and legalizer for ret void.

This is largely based on the initial GlobalISel patch for RISCV
(https://reviews.llvm.org/D65219).

Diff Detail

Event Timeline

kbarton created this revision.Jul 2 2020, 3:28 PM
kbarton added reviewers: echristo, Restricted Project.Jul 2 2020, 3:32 PM
kbarton added reviewers: dsanders, bogner.
arsenm added a subscriber: arsenm.Jul 2 2020, 6:15 PM
arsenm added inline comments.
llvm/lib/Target/PowerPC/PPC.h
88–89

These should be const?

llvm/lib/Target/PowerPC/PPCInstructionSelector.cpp
40

I'm pretty sure you don't need these and all the other places that override this are dead code

kbarton updated this revision to Diff 275409.Jul 3 2020, 9:06 AM

Add const to all parameters of createPPCInstructionSelector.

kbarton updated this revision to Diff 275411.Jul 3 2020, 9:10 AM

Refreshing diff.

kbarton updated this revision to Diff 275412.Jul 3 2020, 9:15 AM

Refreshing diff

kbarton marked 2 inline comments as done.Jul 3 2020, 9:19 AM

I had some problems refreshing the patch using arcanist. I *think* I have fixed everything now, but if things look odd please let me know.

llvm/lib/Target/PowerPC/PPC.h
88–89

Yup, thanks for catching this. Fixed.

llvm/lib/Target/PowerPC/PPCInstructionSelector.cpp
40

I don't follow this.
Both select and getName seem to be required - getName is needed by the base InstructionSelector implementation in GlobalISel; select is needed by the PPCGenGlobalISel.inc file generated below.

It is entirely possible I'm doing something incorrect though. Could you explain some more?

lenary removed a subscriber: lenary.Jul 3 2020, 9:40 AM

AArch64 has a sub-directory for GlobalISel related things:
https://reviews.llvm.org/D81116

madhur13490 added inline comments.
llvm/lib/Target/PowerPC/PPCCallLowering.cpp
42

Why not do "return VRegs.empty()"? You can add a comment if it needs readability.

llvm/lib/Target/PowerPC/PPCInstructionSelector.cpp
23

May be ppc-gisel better?

tschuett added inline comments.Jul 3 2020, 2:32 PM
llvm/lib/Target/PowerPC/PPCLegalizerInfo.h
21

Legalizer or register banks?

arsenm added inline comments.Jul 6 2020, 10:45 AM
llvm/lib/Target/PowerPC/PPCInstructionSelector.cpp
40

Oh right, this isn't the direct pass. I think manual getName overrides are dead code on passes and set by the INITIALIZE_PASS* macros

kbarton updated this revision to Diff 275814.Jul 6 2020, 1:04 PM
kbarton marked 6 inline comments as done.
  • Address review comments - minor clean in PPC::lowerFormalArguments and update comments.

AArch64 has a sub-directory for GlobalISel related things:
https://reviews.llvm.org/D81116

I like this idea. I will do the refactoring in a subsequent commit and update the patch, so if others do not like it it's easy to undo.

llvm/lib/Target/PowerPC/PPCInstructionSelector.cpp
23

Yes, this is a better idea. Will change it.

40

Yes, that could be. I haven't looked at the implementation for how it works on passes.

Are you OK marking this as done?

llvm/lib/Target/PowerPC/PPCLegalizerInfo.h
21

Good catch. Updated to legalizer.

arsenm added a comment.Jul 6 2020, 1:11 PM

AArch64 has a sub-directory for GlobalISel related things:
https://reviews.llvm.org/D81116

I like this idea. I will do the refactoring in a subsequent commit and update the patch, so if others do not like it it's easy to undo.

I don't particularly like the AArch64 split. There isn't an entirely clean break between globalisel parts and the rest of the backend. The other target subdirectories are used for places where there's a separate library

kbarton updated this revision to Diff 275823.Jul 6 2020, 1:39 PM
  • Put all new GlobalISel related files into new GISel subdirectory.

Pardon my cmake, but don't you need a CMakeLists.txt in the GISel sub-directory?

Pardon my cmake, but don't you need a CMakeLists.txt in the GISel sub-directory?

I thought I would too, but I added GISel/ prefix to the CMake in the PowerPC directory, which works.
This is the way it was done in the Aarch64 patch that did the refactoring.

That said, I am by no means a cmake expert, so if having a separate CMakeLists.txt file in the GISel directory is "better", I can add one there also.

arsenm added a comment.Jul 7 2020, 9:13 AM

Pardon my cmake, but don't you need a CMakeLists.txt in the GISel sub-directory?

I thought I would too, but I added GISel/ prefix to the CMake in the PowerPC directory, which works.
This is the way it was done in the Aarch64 patch that did the refactoring.

That said, I am by no means a cmake expert, so if having a separate CMakeLists.txt file in the GISel directory is "better", I can add one there also.

It's not a separate library, so it's not better. Things are not layered to make this possible

kbarton updated this revision to Diff 284039.Aug 7 2020, 1:49 PM

Fix warnings from clang-tidy and clang-format and rebase to latest version of master.

arsenm accepted this revision.Aug 7 2020, 2:03 PM
arsenm added inline comments.
llvm/lib/Target/PowerPC/PPCSubtarget.h
171

InlineAsmLowering is another boilerplate field, but you can also leave that for later

This revision is now accepted and ready to land.Aug 7 2020, 2:03 PM
kbarton updated this revision to Diff 286131.Aug 17 2020, 1:35 PM
  • Add const to all parameters of createPPCInstructionSelector.
  • Address review comments - minor clean in PPC::lowerFormalArguments and update comments.
  • Put all new GlobalISel related files into new GISel subdirectory.
  • Fix warnings from clang-tidy and clang-format.
  • Remove unnecessary whitespace.
arsenm added inline comments.Aug 17 2020, 1:50 PM
llvm/test/CodeGen/PowerPC/GlobalISel/legalize-ret.mir
2

Don't need -global-isel if directly running MIR passes

This revision was landed with ongoing or failed builds.Sep 10 2020, 9:58 AM
This revision was automatically updated to reflect the committed changes.