This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][GISel] support 64 bit load/store
ClosedPublic

Authored by shchenz on Sep 27 2022, 11:41 PM.

Details

Summary

This patch supports 64 bit G_LOAD/G_STORE.

In PPCRegisterBankInfo.cpp, some codes are from AArch64 target. Since PPC Global ISel is still in an early stage, I just made another copy in PPC instead of putting the sharing codes to a common place. Will do the refactor later.

Diff Detail

Event Timeline

shchenz created this revision.Sep 27 2022, 11:41 PM
shchenz requested review of this revision.Sep 27 2022, 11:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 11:41 PM
tschuett added inline comments.
llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp
139

Did you say 64bit?

arsenm added inline comments.Sep 28 2022, 8:33 AM
llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp
266–267

What's the problem with importing the tablegen patterns? This looks pretty simple?

281

Shouldn't create one off MachineIRBuilders

llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp
138–139

This cannot happen

155–157

Should put in utils

170

I wouldn't really call FNEG or FABS FP opcodes

shchenz updated this revision to Diff 466344.Oct 8 2022, 10:30 PM
shchenz marked 4 inline comments as done.

address comments

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

Sure, I can remove this. There is no user for this now.

266–267

PPC does not have a single rule for the simple pattern below in td files, it uses one rule to match all patterns in the PPCTargetLowering.

llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp
155–157

I will post another NFC patch to refactor these functions.

170

Sorry, I do not quite understand. Do you mean we should not add G_FNEG and G_FABS here? But these two opcodes are floating pointer operations, with one floating point input and one floating point output?

arsenm requested changes to this revision.Nov 16 2022, 10:11 AM
arsenm added inline comments.
llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp
117

I think the intent is the call here shouldn't need to look at uses

137

This call doesn't make any sense, you're just getting a pointer to MI with more steps

170

They don't really have floating point semantics. They're non-canonicalizing bit operations, but maybe it makes sense for PPC instructions?

This revision now requires changes to proceed.Nov 16 2022, 10:11 AM
shchenz marked an inline comment as done.Nov 18 2022, 12:38 AM
shchenz added inline comments.
llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp
117

Thanks for review.

I am a little confused by this comment. Do we have any new design for the bank selection for G_LOAD/G_STORE. As we don't have G_FLOAD for float type, to me if we need to distinguish float load and fix point load, we have to go through the G_LOAD's users to decide the load instruction's register bank. On PPC, we have fix point load instructions ld/lwa(and other for other width), and we also have instructions lfd/lfs for float point load.

This is the same handling with AArch4 target for G_LOAD/G_STORE node. See https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AArch64/GISel/AArch64RegisterBankInfo.cpp#L779-L792

137

Similar with G_LOAD. With current Generic MIR infra, on PPC, we have to know what instruction defines the operand 0 of the MI, float point or fixed point. Operand 0 is the content stored to the memory, right?

170

Yes, we have floating point abs/neg instrutions, fabs and fneg on PPC.

shchenz requested review of this revision.Nov 18 2022, 4:34 AM
shchenz marked an inline comment as done.
arsenm added inline comments.Nov 18 2022, 8:58 AM
llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp
117

I guess I thought the point of all the mapping costs were to let the pass look at the uses. I haven't looked that closely at this usage of regbankselect

137

Operand 0 is the result operand. Operand 1 is the stored value

shchenz marked an inline comment as done.Nov 20 2022, 5:44 PM
shchenz added inline comments.
llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp
137

hmm, I dump the operands for the MI G_STORE %1:_(s64), %0:_(p0) :: (store (s64) into %ir.p), the operand 0 is %1:_(s64), operand 1 is %0:_(p0).

the ll is:

define void @store_i64(i64* %p) {
entry:
  store i64 100, i64* %p, align 8
  ret void
}

MIR before reg bank selection is:

# Machine code for function store_i64: IsSSA, TracksLiveness, Legalized
Function Live Ins: $x3

bb.1.entry:
  liveins: $x3
  %0:_(p0) = COPY $x3
  %1:_(s64) = G_CONSTANT i64 100
  G_STORE %1:_(s64), %0:_(p0) :: (store (s64) into %ir.p)
  BLR8 implicit $lr8, implicit $rm

# End machine code for function store_i64.
arsenm requested changes to this revision.Nov 22 2022, 6:52 AM

I'd like to see more test coverage of the various use scans for regbankselect. In particular multiple uses, and mixed type uses between FP and integer

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

The tests aren't covering these various use checks

137

Sorry, I thought I was looking at load

llvm/test/CodeGen/PowerPC/GlobalISel/load-store-64bit.ll
6

New tests should use opaque pointers

This revision now requires changes to proceed.Nov 22 2022, 6:52 AM
shchenz updated this revision to Diff 477352.Nov 22 2022, 5:29 PM
shchenz marked an inline comment as done.

address @arsenm comments

shchenz marked 2 inline comments as done.Nov 22 2022, 5:31 PM
shchenz added inline comments.
llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp
117

Sorry, I meant to add more cases, but seems now there are not too many generic opcodes support on PPC target for globalisel, for example G_FPTOSI/G_SITOFP...

arsenm added inline comments.Nov 28 2022, 7:00 AM
llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp
278–280

The verifier will catch this with correctly defined legalizer rules

293

Don't bother with this assert, the verifier checks this

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

Should add a few simple ones in a pre-commit to use in tests here

shchenz marked 4 inline comments as done.Nov 28 2022, 11:53 PM
shchenz added inline comments.
llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp
117

pre-commit cases will crash because there is no load/store support for PPC GISEL . Do you mean I pre-commit these crash ones guarded with not --crash?

shchenz updated this revision to Diff 478471.Nov 28 2022, 11:54 PM
shchenz marked an inline comment as done.

address comments

arsenm added inline comments.Nov 29 2022, 12:56 PM
llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp
117

No, I mean do a simple patch to add support for a few more FP operations. You have working arg and return, so it's not dependent on load/store first

shchenz updated this revision to Diff 478816.Nov 29 2022, 11:25 PM

Make load/store selection be parent of constantFP selection patch

amyk added inline comments.Nov 30 2022, 7:01 AM
llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp
127

Minor nit: Capitalize the first word of the variable to adhere to the LLVM coding guidelines.

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

nit: Maybe we can say Unsupported load type!

But also a question - I know there was a comment earlier stating that the assert in the instruction selector wasn't needed because the verifier checks that kind of stuff, but is this the same for the register bank selection pass?

arsenm added inline comments.Nov 30 2022, 8:22 AM
llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp
115

Yes, there's an assert for legal operations before RegBankSelect

shchenz updated this revision to Diff 479119.Nov 30 2022, 5:22 PM

address comments

shchenz updated this revision to Diff 479551.Dec 2 2022, 1:32 AM

add more cases

arsenm added inline comments.Dec 5 2022, 6:56 AM
llvm/lib/Target/PowerPC/GISel/PPCLegalizerInfo.cpp
50

This is missing the pointer operand. I'm surprised this doesn't fail anywhere

shchenz updated this revision to Diff 480347.Dec 5 2022, 11:56 PM

use all operands for legalizing G_LOAD/G_STORE

shchenz marked an inline comment as done.Dec 6 2022, 12:32 AM
shchenz added inline comments.
llvm/lib/Target/PowerPC/GISel/PPCLegalizerInfo.cpp
50

Good catch. The previous version only matches the first operand. And our cases are all have valid first operand and also the other operands.
I tried to add some MIR cases that pass with previous version but failed with the new update version, for example, making the second operand as scalar type instead of pointer type or making the MI have no memory operand, but both failed. Seems G_LOAD/G_STORE has its own constraints, for example, the second operand must be a pointer and there must be memory operand. A 32-bit pointer would be a possible case, but that is not the case PPC currently trying to support, for now, only PPC64LE is supported.

arsenm added inline comments.Dec 6 2022, 8:23 PM
llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp
115

The tests don't involve different use lists that would fail this check

arsenm requested changes to this revision.Dec 7 2022, 6:42 AM
This revision now requires changes to proceed.Dec 7 2022, 6:42 AM
shchenz updated this revision to Diff 481961.Dec 11 2022, 6:02 PM
shchenz marked an inline comment as done.

add test cases with more than one use for the load

shchenz marked an inline comment as done.Dec 11 2022, 6:09 PM
shchenz added inline comments.
llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp
115

updated.

arsenm accepted this revision.Dec 11 2022, 7:30 PM

LGTM with nit

llvm/test/CodeGen/PowerPC/GlobalISel/load-store-64bit.ll
89

Use named values in tests

This revision is now accepted and ready to land.Dec 11 2022, 7:30 PM
shchenz marked 2 inline comments as done.Dec 12 2022, 4:19 AM
shchenz added inline comments.
llvm/test/CodeGen/PowerPC/GlobalISel/load-store-64bit.ll
89

Sure, will change this in commit.

This revision was landed with ongoing or failed builds.Dec 12 2022, 4:23 AM
This revision was automatically updated to reflect the committed changes.
shchenz marked an inline comment as done.