This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Extend GlobalISel implementation to emit and/or/xor.
ClosedPublic

Authored by Kai on Jun 10 2022, 12:42 PM.

Details

Summary

Adds some more code to GlobalISel to enable instruction selection for and/or/xor.

  • Makes G_IMPLICIT_DEF, G_CONSTANT, G_AND, G_OR, G_XOR legal for 64bit register size.
  • Implement lowerReturn in CallLowering
  • Provides mapping of the operands to register banks.
  • Adds register info to G_COPY operands.

The utility functions are all only implemented so far to support this use case.
Especially the functions in PPCGenRegisterBankInfo.def are too simple for
general use.

Diff Detail

Event Timeline

Kai created this revision.Jun 10 2022, 12:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2022, 12:42 PM
Kai requested review of this revision.Jun 10 2022, 12:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2022, 12:42 PM

Nice! My only comment is that my need some more space in PPCLegalizerInfo. You will have to add a lot more code, see e.g. AARCH64:
https://github.com/llvm/llvm-project/blob/275b2e5243639268033d6f0183b9b3b1a30574be/llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp#L41

Kai added a comment.Jun 10 2022, 1:23 PM

Nice! My only comment is that my need some more space in PPCLegalizerInfo. You will have to add a lot more code, see e.g. AARCH64:
https://github.com/llvm/llvm-project/blob/275b2e5243639268033d6f0183b9b3b1a30574be/llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp#L41

Sure! This is only meant as a first improvement to the existing infrastructure, to see some real instructions generated. E.g. the register bank info is also very "thin" and needs much more work.

Kai updated this revision to Diff 436039.Jun 10 2022, 2:01 PM
Kai edited the summary of this revision. (Show Details)
  • Fixed a wrong comment
  • Legalize G_CONSTANT
  • Finish test case for nand
Kai updated this revision to Diff 436543.Jun 13 2022, 1:33 PM
  • clamp scalars
  • more tests
Kai updated this revision to Diff 437269.Jun 15 2022, 11:37 AM
  • Add G8RC_NOX0 to register bank
  • Add new test to check lowering to GMIR
arsenm added inline comments.Jun 17 2022, 4:29 PM
llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp
78–88

I don't see the point of this function. There's no guessing involved and you're returning a static result anyway

94

You still need to at least constrain the source register if it's virtual. You probably should have some MIR tests for every permutation of physical / virtual register copies

llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp
84–87

You shouldn't need to guard these. getRegBank should do the right thing

Kai updated this revision to Diff 438814.Jun 21 2022, 1:29 PM
  • Removed function guessRegClass()
  • selectCopy() now constraints both src and dst
  • Simplified case COPY: in RegisterBankInfo
Kai marked 2 inline comments as done.Jun 21 2022, 1:41 PM

Thanks for the comments!

Considering your comments on D128106 I guess it makes also sense to remove the ppc-irtranslator test and extend the ppc-isel-logical.ll test with the various non-legal types.

llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp
94

Is this actually needed? I added the code but I see no change.
If I compare this with mipsel-linux:

Before instruction-selection:

  %2:gprb(s32) = COPY $a0
...
  $v0 = COPY %13(s32)

and after instruction-selection:

  %2:gpr32 = COPY $a0
...
  $v0 = COPY %13

So during instruction selection, for the destination virtual register, the register bank is replaced with the actual register class. But in the other case, the virtual source register has no constraint after the instruction-select.
I get similar output when also constraining the source virtual register.

nemanjai added inline comments.Jul 1 2022, 1:20 AM
llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp
41

Is it not possible to add tests at this time that actually extend the return value from i32 (for both sixgnext, zeroext)?

94–99

I understand that you implemented this by looking at other targets that already have this implemented. However, I think it would help other PPC developers (as well as anyone implementing this for a future target) if we add a short comment before each statement here describing what the function call does or what the purpose of the object is.

llvm/lib/Target/PowerPC/GISel/PPCGenRegisterBankInfo.def
11

Can you please add a comment as to why it is not yet auto generated?

27–29

Why are there 3 entries here? Thorough documentation at this stage will make it significantly easier when the time comes to refactor the implementation and add automatic generation, etc.

Kai updated this revision to Diff 442361.Jul 5 2022, 11:14 AM
  • Add comments
  • Remove test ppc-irtranslator.ll
Kai marked 3 inline comments as done.Jul 5 2022, 11:17 AM
Kai added inline comments.
llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp
41

It's not yet possible. signext requires a legal G_SEXT instruction.
zeroext either requires handling G_CONSTANT (to clear the upper bits), or a combiner (redundant_and) to remove the and instruction used to clear the bits.

Kai added a reviewer: Restricted Project.
shchenz added inline comments.Sep 5 2022, 1:31 AM
llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp
80

Maybe we could move this to target independent handling at line 50? getInstrMappingImpl should able to handle case where register bank of one operand of the copy instruction is known.

arsenm added inline comments.Sep 14 2022, 3:15 PM
llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp
94

Most of the time it works, since the use constrain also happens to work for the dest instruction. However there are some cases where this doesn't happen, leaving an unconstrained register. You'll need to have different kinds of uses and defs. I can't think of a nice example at the moment

amyk added inline comments.Sep 14 2022, 9:36 PM
llvm/test/CodeGen/PowerPC/GlobalISel/ppc-isel-logical.ll
1

Maybe we can autogenerate the checks? And perhaps add the -ppc-asm-full-reg-names option (since most of our other test cases also include this)?

Kai updated this revision to Diff 465193.Oct 4 2022, 3:12 PM
  • update selectCopy() to constraint all registers
  • regenerate test using -ppc-asm-full-reg-names option
Kai marked an inline comment as done.Oct 4 2022, 3:13 PM
Kai added inline comments.
llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp
94

Thanks, I see. Changed the code to constrain all registers.

tschuett added inline comments.Oct 4 2022, 3:26 PM
llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp
45

Could you move the implementations out of the anonymous namespace?
https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

Kai updated this revision to Diff 465206.Oct 4 2022, 3:38 PM

Let the generic code handle TargetOpcode::COPY.

llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp
80

Good point, I changed the code.

Kai updated this revision to Diff 465211.Oct 4 2022, 3:50 PM

Moved implementation out of anonymous name space in PPCCallLowering.cpp.

Kai marked 5 inline comments as done.Oct 4 2022, 3:51 PM
shchenz added inline comments.Oct 7 2022, 7:04 PM
llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp
86

There is an approved version of selectCopy() in D132942. We may only need to constrain dest virtual register in the COPY instruction and for the source registers, we can defer them to other uses or defs?

reverse ping, can we move forward for this patch? Thanks. @Kai

Kai updated this revision to Diff 476619.Nov 18 2022, 3:30 PM

Added selectCopy() implementation from D132942

Kai marked an inline comment as done.Nov 18 2022, 3:33 PM

@shchenz Sorry for the delay.
I copied the selectCopy() function from your change. I also added a simplified version of getRegClass(). That is hopefully good enough.

shchenz accepted this revision as: shchenz.Nov 19 2022, 6:40 PM

LGTM. Thanks.

This revision is now accepted and ready to land.Nov 19 2022, 6:40 PM