[PATCH] D132942: [PowerPC][GISel]add support for float point arithmetic operations

[Nemanja Ivanovic via Phabricator via llvm-commits]

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

This patch may not actually require code changes, but I would like to have the questions I posted answered before I feel confident letting it go in.



================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:93
+
 static bool selectCopy(MachineInstr &I, const TargetInstrInfo &TII,
                        MachineRegisterInfo &MRI, const TargetRegisterInfo &TRI,
----------------
It seems that this function does something sensible in 3 of 4 possible situations:


  # Both SrcReg and DstReg are physical regs, just return `true`
  # DstReg is virtual and SrcReg is physical, constrain DstReg
  # DstReg is physical and SrcReg is virtual, constrain SrcReg
  # Both DstReg and SrcReg are virtual, constrain DstReg and leave SrcReg untouched

Why do we behave this way for 4. above?



================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp:40-42
+  case PPC::SPILLTOVSRRC_and_VSFRCRegClassID:
+  case PPC::SPILLTOVSRRC_and_VFRCRegClassID:
+  case PPC::SPILLTOVSRRC_and_F4RCRegClassID:
----------------
Why are these classes included? Is it expected that global isel works with `-ppc-enable-gpr-to-vsr-spills`?


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp:57-58
 
   // Try the default logic for non-generic instructions that are either copies
   // or already have some operands assigned to banks.
+  if (!isPreISelGenericOpcode(Opc) || Opc == TargetOpcode::G_PHI) {
----------------
It is not clear from the description why the handling of copies is changing here. Also, it would seem that the comment is no longer valid.


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp:93-96
+    if (Size == 32)
+      OperandsMapping = getValueMapping(PMI_FPR32);
+    else
+      OperandsMapping = getValueMapping(PMI_FPR64);
----------------
```
OperansMapping = getValueMapping(Size == 32 ? PMI_FPR32 : PMI_FPR64);
```


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCRegisterBanks.td:17
+/// Float point Registers
+def FPRRegBank : RegisterBank<"FPR", [VSSRC]>;
----------------
I probably don't understand this well enough, but I find it surprising that the list of register classes only includes `VSSRC` (i.e. all 64 single precision VSX registers). Why not `VSFRC` for double precision - and why not the subclasses `F8RC, F4RC`?


================
Comment at: llvm/test/CodeGen/PowerPC/GlobalISel/float-arithmetic.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=powerpc64le-unknown-linux-gnu -global-isel -o - < %s | FileCheck %s
+
----------------
What would happen if `-mattr=-vsx` was added to this invocation? A crash? An error? We still get VSX instructions? We defer to SDAG ISel?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132942