[PATCH] D134792: [PowerPC][GISel] support 64 bit load/store

[Matt Arsenault via Phabricator via llvm-commits]

arsenm requested changes to this revision.
arsenm added a comment.
This revision now requires changes to proceed.

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



================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp:123
+    // Check if that load feeds fp instructions.
+    if (any_of(MRI.use_nodbg_instructions(MI.getOperand(0).getReg()),
+               [&](const MachineInstr &UseMI) {
----------------
arsenm wrote:
> shchenz wrote:
> > arsenm wrote:
> > > I think the intent is the call here shouldn't need to look at uses
> > 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
> 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 
The tests aren't covering these various use checks 


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp:143
+    // Check if the store is fed by fp instructions.
+    MachineInstr *DefMI = MRI.getVRegDef(MI.getOperand(0).getReg());
+    if (onlyDefinesFP(*DefMI, MRI, TRI))
----------------
shchenz wrote:
> arsenm wrote:
> > shchenz wrote:
> > > arsenm wrote:
> > > > This call doesn't make any sense, you're just getting a pointer to MI with more steps 
> > > 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?
> > Operand 0 is the result operand. Operand 1 is the stored value 
> 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.
> ```
Sorry, I thought I was looking at load 


================
Comment at: llvm/test/CodeGen/PowerPC/GlobalISel/load-store-64bit.ll:5
+
+define i64 @load_i64(i64* %p) {
+; CHECK-LABEL: load_i64:
----------------
New tests should use opaque pointers 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134792/new/

https://reviews.llvm.org/D134792