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

[ChenZheng via Phabricator via llvm-commits]

shchenz marked an inline comment as done.
shchenz added inline comments.


================
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:
> 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


================
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))
----------------
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?


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp:168
+  case TargetOpcode::G_FNEARBYINT:
+  case TargetOpcode::G_FNEG:
+  case TargetOpcode::G_FCOS:
----------------
arsenm wrote:
> shchenz wrote:
> > arsenm wrote:
> > > I wouldn't really call FNEG or FABS FP opcodes
> > 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?
> They don't really have floating point semantics. They're non-canonicalizing bit operations, but maybe it makes sense for PPC instructions?
Yes, we have floating point abs/neg instrutions, fabs and fneg on PPC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134792