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

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
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:
----------------
nemanjai wrote:
> Why are these classes included? Is it expected that global isel works with `-ppc-enable-gpr-to-vsr-spills`?
This is from the auto-generated td file `PPCGenRegisterBank.inc` for register bank `FPRRegBankID`. This function should be TableGen'ed.


================
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) {
----------------
nemanjai wrote:
> 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.
See comment https://reviews.llvm.org/D127530#3769630
I will update the comments later.


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCRegisterBanks.td:17
+/// Float point Registers
+def FPRRegBank : RegisterBank<"FPR", [VSSRC]>;
----------------
nemanjai wrote:
> 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`?
This is because the register bank contains the specified register classes and all their sub register classes. So we just need to specify the largest register class. In PPCRegisterInfo.td, we have:
```
def VSFRC : RegisterClass<"PPC", [f64], 64, (add F8RC, VFRC)>;
def VSSRC : RegisterClass<"PPC", [f32], 32, (add VSFRC)>;
```

So adding `VSSRC` will automatically make `FPRRegBank` contain `VSFRC`and further contain `F8RC` and `VFRC` because they are sub register classes of `VSFRC`.


================
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
+
----------------
nemanjai wrote:
> 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?
It is ok to compile for `-mattr=-vsx` and `fadds` will be emitted as expected. Generic global-isel node `G_FADD` is mapped to DAG node `fadd`. The `fadd` will be selected in ppcinstr.*.td files like DAG instruction does, i.e., select `xsaddsp` first and then `fadds` if vsx is not available.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132942