[PowerPC][GISel]add support for float point arithmetic operations

[shchenz]

Summary

This is based on https://reviews.llvm.org/D127530

Add global isel support for {G_FADD, G_FSUB, G_FMUL, G_FDIV}

[Kai]

LGTM with the message in the assert changed.

llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp

92

[nemanjai]

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.

llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp

93

It seems that this function does something sensible in 3 of 4 possible situations:

1. Both SrcReg and DstReg are physical regs, just return true
2. DstReg is virtual and SrcReg is physical, constrain DstReg
3. DstReg is physical and SrcReg is virtual, constrain SrcReg
4. Both DstReg and SrcReg are virtual, constrain DstReg and leave SrcReg untouched

Why do we behave this way for 4. above?

llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp

40–42

Why are these classes included? Is it expected that global isel works with -ppc-enable-gpr-to-vsr-spills?

57–59

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.

93–96

OperansMapping = getValueMapping(Size == 32 ? PMI_FPR32 : PMI_FPR64);

llvm/lib/Target/PowerPC/GISel/PPCRegisterBanks.td

17

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?

llvm/test/CodeGen/PowerPC/GlobalISel/float-arithmetic.ll

3

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?

[Kai]

llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp

93

Good point. I believe the outlier is case 3. The copy instruction is somewhat special. My understanding is that we need to care only about the DstReg, because the SrcReg is either a physical register or is constraint somewhere else. Since most of the code comes from my PR, my proposal is that I change the code in my PR to

for (const MachineOperand &MO : I.operands()) {
  if (MO.getReg().isPhysical())
    continue;

  const TargetRegisterClass *RC =
          TRI.getConstrainedRegClassForOperand(MO, *MRI);
  if (!RC)
    continue;
  RBI.constrainGenericRegister(MO.getReg(), *RC, *MRI);
}
return true;

This is imho more uniform, and better to read.

[shchenz]

llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp

40–42

This is from the auto-generated td file PPCGenRegisterBank.inc for register bank FPRRegBankID. This function should be TableGen'ed.

57–59

See comment https://reviews.llvm.org/D127530#3769630
I will update the comments later.

llvm/lib/Target/PowerPC/GISel/PPCRegisterBanks.td

17

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 VSFRCand further contain F8RC and VFRC because they are sub register classes of VSFRC.

llvm/test/CodeGen/PowerPC/GlobalISel/float-arithmetic.ll

3

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.

[shchenz]

address comments

llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp

57–59

The comments seems wrong before the change and right now.

[amyk]

llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp

40–42

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

llvm/lib/Target/PowerPC/GISel/PPCRegisterBanks.td

16

nit: Floating point Registers.

llvm/test/CodeGen/PowerPC/GlobalISel/float-arithmetic.ll

3

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.

[arsenm]

llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp

97–111

This won't correctly handle the case where the incoming copy already has a class assigned. This may end up widening the class constraint

[shchenz]

address comments

[shchenz]

Thanks for review @amyk @arsenm . Updated accordingly.

llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp

97–111

Make sense. Updated accordingly. FYI @Kai

llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp

40–42

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.

llvm/test/CodeGen/PowerPC/GlobalISel/float-arithmetic.ll

3

Good idea.

[arsenm]

llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp

112

Most of this function is repeating the work RBI.constrainGenericRegister already does. You just need to get the class implied by the bank with the type, and pass that to RBI.constrainGenericRegister. It figures out if there's a class set or not and handles subclass checks

[shchenz]

address comments

[amyk]

Thanks for answering my questions and addressing the comments I left.

llvm/lib/Target/PowerPC/GISel/PPCGenRegisterBankInfo.def

44–48

Aren't these comments more accurately reflected as the following?

[shchenz]

fix comments

[shchenz]

llvm/lib/Target/PowerPC/GISel/PPCGenRegisterBankInfo.def

44–48

good catch.

[arsenm]

LGTM

[nemanjai]

LGTM.

[shchenz]

rebase

[amyk]

LGTM as well. Thank you!

llvm/lib/Target/PowerPC/GISel/PPCGenRegisterBankInfo.def

 ///  derived from register bank definition. Not yet implemented.
 //===----------------------------------------------------------------------===//
 
 namespace llvm {
 RegisterBankInfo::PartialMapping PPCGenRegisterBankInfo::PartMappings[]{
     /* StartIdx, Length, RegBank */
     // 0: GPR 64-bit value.
     {0, 64, PPC::GPRRegBank},
+    // 1: FPR 32-bit value
+    {0, 32, PPC::FPRRegBank},
+    // 2: FPR 64-bit value
+    {0, 64, PPC::FPRRegBank},
 };
 
 // ValueMappings.
 // Pointers to the entries in this array are returned by getValueMapping() and
 // getCopyMapping().
 //
 // The array has the following structure:
 // - At index 0 is the invalid entry.
 // - After that, the mappings for the register types from PartialMappingIdx
 //   follow. Each mapping consists of 3 entries, which is needed to cover
 //   3-operands instructions.
 // - Last, mappings for cross-register bank moves follow. Since COPY has only
 //   2 operands, a mapping consists of 2 entries.
 RegisterBankInfo::ValueMapping PPCGenRegisterBankInfo::ValMappings[]{
     /* BreakDown, NumBreakDowns */
     // 0: invalid
     {nullptr, 0},
     // 1: GPR 64-bit value.
     {&PPCGenRegisterBankInfo::PartMappings[PMI_GPR64 - PMI_Min], 1},
     {&PPCGenRegisterBankInfo::PartMappings[PMI_GPR64 - PMI_Min], 1},
     {&PPCGenRegisterBankInfo::PartMappings[PMI_GPR64 - PMI_Min], 1},
+    // 4: FPR 32-bit value.
+    {&PPCGenRegisterBankInfo::PartMappings[PMI_FPR32 - PMI_Min], 1},
+    {&PPCGenRegisterBankInfo::PartMappings[PMI_FPR32 - PMI_Min], 1},
+    {&PPCGenRegisterBankInfo::PartMappings[PMI_FPR32 - PMI_Min], 1},
+    // 7: FPR 64-bit value.
+    {&PPCGenRegisterBankInfo::PartMappings[PMI_FPR64 - PMI_Min], 1},
+    {&PPCGenRegisterBankInfo::PartMappings[PMI_FPR64 - PMI_Min], 1},
+    {&PPCGenRegisterBankInfo::PartMappings[PMI_FPR64 - PMI_Min], 1},
 };
 
 // TODO Too simple!
 const RegisterBankInfo::ValueMapping *
 PPCGenRegisterBankInfo::getValueMapping(PartialMappingIdx RBIdx) {
   assert(RBIdx != PartialMappingIdx::PMI_None && "No mapping needed for that");
 
   unsigned ValMappingIdx = RBIdx - PMI_Min;

llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp

 #include "PPCGenGlobalISel.inc"
 #undef GET_GLOBALISEL_PREDICATES_INIT
 #define GET_GLOBALISEL_TEMPORARIES_INIT
 #include "PPCGenGlobalISel.inc"
 #undef GET_GLOBALISEL_TEMPORARIES_INIT
 {
 }
 
+static const TargetRegisterClass *getRegClass(LLT Ty, const RegisterBank *RB) {
+  if (RB->getID() == PPC::GPRRegBankID) {
+    if (Ty.getSizeInBits() == 64)
+      return &PPC::G8RCRegClass;
+  }
+  if (RB->getID() == PPC::FPRRegBankID) {
+    if (Ty.getSizeInBits() == 32)
+      return &PPC::F4RCRegClass;
+    if (Ty.getSizeInBits() == 64)
+      return &PPC::F8RCRegClass;
+  }
+
+  llvm_unreachable("Unknown RegBank!");
+}
+
 static bool selectCopy(MachineInstr &I, const TargetInstrInfo &TII,
                        MachineRegisterInfo &MRI, const TargetRegisterInfo &TRI,
                        const RegisterBankInfo &RBI) {
   Register DstReg = I.getOperand(0).getReg();
-  Register SrcReg = I.getOperand(1).getReg();
 
-  if (!Register::isPhysicalRegister(DstReg))
-    if (!RBI.constrainGenericRegister(DstReg, PPC::G8RCRegClass, MRI))
-      return false;
-  if (!Register::isPhysicalRegister(SrcReg))
-    if (!RBI.constrainGenericRegister(SrcReg, PPC::G8RCRegClass, MRI))
-      return false;
+  if (DstReg.isPhysical())
+    return true;
+
+  const RegisterBank *DstRegBank = RBI.getRegBank(DstReg, MRI, TRI);
+  const TargetRegisterClass *DstRC =
+      getRegClass(MRI.getType(DstReg), DstRegBank);
+
+  // No need to constrain SrcReg. It will get constrained when we hit another of
+  // its use or its defs.
+  // Copies do not have constraints.
+  if (!RBI.constrainGenericRegister(DstReg, *DstRC, MRI)) {
+    LLVM_DEBUG(dbgs() << "Failed to constrain " << TII.getName(I.getOpcode())
+                      << " operand\n");
+  }
 
   return true;
 }
 
 bool PPCInstructionSelector::select(MachineInstr &I) {
   auto &MBB = *I.getParent();
   auto &MF = *MBB.getParent();
   auto &MRI = MF.getRegInfo();

llvm/lib/Target/PowerPC/GISel/PPCLegalizerInfo.cpp

 
 #define DEBUG_TYPE "ppc-legalinfo"
 
 using namespace llvm;
 using namespace LegalizeActions;
 
 PPCLegalizerInfo::PPCLegalizerInfo(const PPCSubtarget &ST) {
   using namespace TargetOpcode;
+  const LLT S32 = LLT::scalar(32);
   const LLT S64 = LLT::scalar(64);
   getActionDefinitionsBuilder(G_IMPLICIT_DEF).legalFor({S64});
   getActionDefinitionsBuilder(G_CONSTANT)
       .legalFor({S64})
       .clampScalar(0, S64, S64);
   getActionDefinitionsBuilder({G_AND, G_OR, G_XOR})
       .legalFor({S64})
       .clampScalar(0, S64, S64);
+
+  getActionDefinitionsBuilder({G_FADD, G_FSUB, G_FMUL, G_FDIV})
+      .legalFor({S32, S64});
+
   getLegacyLegalizerInfo().computeTables();
 }

llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.h

 namespace llvm {
 class TargetRegisterInfo;
 
 class PPCGenRegisterBankInfo : public RegisterBankInfo {
 protected:
   enum PartialMappingIdx {
     PMI_None = -1,
     PMI_GPR64 = 1,
+    PMI_FPR32 = 2,
+    PMI_FPR64 = 3,
     PMI_Min = PMI_GPR64,
   };
 
   static RegisterBankInfo::PartialMapping PartMappings[];
   static RegisterBankInfo::ValueMapping ValMappings[];
   static PartialMappingIdx BankIDToCopyMapIdx[];
 
   /// Get the pointer to the ValueMapping representing the RegisterBank

llvm/lib/Target/PowerPC/GISel/PPCRegisterBankInfo.cpp

 const RegisterBank &
 PPCRegisterBankInfo::getRegBankFromRegClass(const TargetRegisterClass &RC,
                                             LLT Ty) const {
   switch (RC.getID()) {
   case PPC::G8RCRegClassID:
   case PPC::G8RC_NOX0RegClassID:
   case PPC::G8RC_and_G8RC_NOX0RegClassID:
     return getRegBank(PPC::GPRRegBankID);
+  case PPC::VSFRCRegClassID:
+  case PPC::SPILLTOVSRRC_and_VSFRCRegClassID:
+  case PPC::SPILLTOVSRRC_and_VFRCRegClassID:
+  case PPC::SPILLTOVSRRC_and_F4RCRegClassID:
+  case PPC::F8RCRegClassID:
+  case PPC::VFRCRegClassID:
+  case PPC::VSSRCRegClassID:
+  case PPC::F4RCRegClassID:
+    return getRegBank(PPC::FPRRegBankID);
   default:
     llvm_unreachable("Unexpected register class");
   }
 }
 
 const RegisterBankInfo::InstructionMapping &
 PPCRegisterBankInfo::getInstrMapping(const MachineInstr &MI) const {
   const unsigned Opc = MI.getOpcode();
 
   // Try the default logic for non-generic instructions that are either copies
   // or already have some operands assigned to banks.
-  if ((Opc != TargetOpcode::COPY && !isPreISelGenericOpcode(Opc)) ||
-      Opc == TargetOpcode::G_PHI) {
+  if (!isPreISelGenericOpcode(Opc) || Opc == TargetOpcode::G_PHI) {
     const RegisterBankInfo::InstructionMapping &Mapping =
         getInstrMappingImpl(MI);
     if (Mapping.isValid())
       return Mapping;
   }
 
   const MachineFunction &MF = *MI.getParent()->getParent();
   const MachineRegisterInfo &MRI = MF.getRegInfo();
switch (Opc) {
     // Bitwise ops.
   case TargetOpcode::G_AND:
   case TargetOpcode::G_OR:
   case TargetOpcode::G_XOR:
     assert(NumOperands <= 3 &&
            "This code is for instructions with 3 or less operands");
     OperandsMapping = getValueMapping(PMI_GPR64);
     break;
-  case TargetOpcode::G_CONSTANT:
-    OperandsMapping = getOperandsMapping({getValueMapping(PMI_GPR64), nullptr});
-    break;
-  case TargetOpcode::COPY: {
-    Register DstReg = MI.getOperand(0).getReg();
+  case TargetOpcode::G_FADD:
+  case TargetOpcode::G_FSUB:
+  case TargetOpcode::G_FMUL:
+  case TargetOpcode::G_FDIV: {
     Register SrcReg = MI.getOperand(1).getReg();
-    const RegisterBank *DstRB = getRegBank(DstReg, MRI, TRI);
-    const RegisterBank *SrcRB = getRegBank(SrcReg, MRI, TRI);
-    if (!DstRB)
-      DstRB = SrcRB;
-    else if (!SrcRB)
-      SrcRB = DstRB;
-    assert(DstRB && SrcRB && "Both RegBank were nullptr");
-    unsigned Size = getSizeInBits(DstReg, MRI, TRI);
-    Cost = copyCost(*DstRB, *SrcRB, Size);
-    OperandsMapping = getCopyMapping(DstRB->getID(), SrcRB->getID(), Size);
-    // We only care about the mapping of the destination.
-    NumOperands = 1;
+    unsigned Size = getSizeInBits(SrcReg, MRI, TRI);
+
+    assert((Size == 32 || Size == 64) && "Unsupported floating point types!\n");
+    OperandsMapping = getValueMapping(Size == 32 ? PMI_FPR32 : PMI_FPR64);
     break;
   }
+  case TargetOpcode::G_CONSTANT:
+    OperandsMapping = getOperandsMapping({getValueMapping(PMI_GPR64), nullptr});
+    break;
   default:
     return getInvalidInstructionMapping();
   }
 
   return getInstructionMapping(MappingID, Cost, OperandsMapping, NumOperands);
 }
 
 RegisterBankInfo::InstructionMappings
 PPCRegisterBankInfo::getInstrAlternativeMappings(const MachineInstr &MI) const {
   // TODO Implement.
   return RegisterBankInfo::getInstrAlternativeMappings(MI);
 }

llvm/lib/Target/PowerPC/GISel/PPCRegisterBanks.td

 //===-- PPCRegisterBanks.td - Describe the PPC Banks -------*- tablegen -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// Define the PPC register banks used for GlobalISel.
 ///
 //===----------------------------------------------------------------------===//
 
 /// General Purpose Registers
 def GPRRegBank : RegisterBank<"GPR", [G8RC, G8RC_NOX0]>;
+/// Floating point Registers
+def FPRRegBank : RegisterBank<"FPR", [VSSRC]>;

llvm/test/CodeGen/PowerPC/GlobalISel/float-arithmetic.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=powerpc64le-unknown-linux-gnu -global-isel -o - \
+; RUN:   -ppc-vsr-nums-as-vr -ppc-asm-full-reg-names < %s | FileCheck %s
+
+define float @float_add(float %a, float %b) {
+; CHECK-LABEL: float_add:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    xsaddsp f1, f1, f2
+; CHECK-NEXT:    blr
+entry:
+  %add = fadd float %a, %b
+  ret float %add
+}
+
+define double @double_add(double %a, double %b) {
+; CHECK-LABEL: double_add:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    xsadddp f1, f1, f2
+; CHECK-NEXT:    blr
+entry:
+  %add = fadd double %a, %b
+  ret double %add
+}
+
+define float @float_sub(float %a, float %b) {
+; CHECK-LABEL: float_sub:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    xssubsp f1, f1, f2
+; CHECK-NEXT:    blr
+entry:
+  %sub = fsub float %a, %b
+  ret float %sub
+}
+
+define float @float_mul(float %a, float %b) {
+; CHECK-LABEL: float_mul:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    xsmulsp f1, f1, f2
+; CHECK-NEXT:    blr
+entry:
+  %mul = fmul float %a, %b
+  ret float %mul
+}
+
+define float @float_div(float %a, float %b) {
+; CHECK-LABEL: float_div:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    xsdivsp f1, f1, f2
+; CHECK-NEXT:    blr
+entry:
+  %div = fdiv float %a, %b
+  ret float %div
+}