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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:373-375
+    assert((LdSt.getMemSizeInBits() ==
+            MRI.getType(LdSt.getReg(0)).getSizeInBits()) &&
+           "load/store size is not same with the defs!");
----------------
The verifier will catch this with correctly defined legalizer rules


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:388
+      I.setDesc(TII.get(NewOpc));
+      assert(I.getOperand(1).isReg() && "load/store address is not a reg!");
+      Register AddrReg = I.getOperand(1).getReg();
----------------
Don't bother with this assert, the verifier checks this


================
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) {
----------------
shchenz wrote:
> arsenm wrote:
> > 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 
> 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`...
Should add a few simple ones in a pre-commit to use in tests here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134792