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

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

Thanks for review @amyk @arsenm . Updated accordingly.



================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:100-114
+    const TargetRegisterClass *RC =
+        getRegClass(MRI.getType(DstReg), RBI.getRegBank(DstReg, MRI, TRI));
+    if (!RBI.constrainGenericRegister(DstReg, *RC, MRI)) {
+      LLVM_DEBUG(dbgs() << "Failed to constrain " << TII.getName(I.getOpcode())
+                        << " dest operand\n");
       return false;
+    }
----------------
arsenm wrote:
> This won't correctly handle the case where the incoming copy already has a class assigned. This may end up widening the class constraint
Make sense. Updated accordingly. FYI @Kai 


================
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:
----------------
amyk wrote:
> shchenz wrote:
> > 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.
> A question that I thought of and was curious about is regarding these register classes and the test cases you added.
> 
> Are those tests only corresponding to a single/certain register classes out of the ones you added? Should we be testing all types of register classes in the tests (if that is even possible)?
> 
> 
The tests I added in this patch are all related to register class `PPC::SPILLTOVSRRC_and_F4RCRegClassID`. This function is called for register bank selection for `COPY` instructions that have a physical register operand. And for floating point physical register, like `$f1`(for both type float and double), `TargetRegisterInfo::getMinimalPhysRegClass()` returns `PPC::SPILLTOVSRRC_and_F4RCRegClassID`.

So I thought it would be difficult to add a case for each register class here. Maybe some register classes listed here can not be returned from `TargetRegisterInfo::getMinimalPhysRegClass()`. Reason I listed them here is I thought the final TableGen'ed version must contain these mappings.


================
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
+
----------------
amyk wrote:
> I was thinking it might be good to include `-ppc-vsr-nums-as-vr -ppc-asm-full-reg-names` like we do in our other test cases.
Good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132942