[PowerPC][GISel] support 64 bit load/store

[shchenz]

Summary

This patch supports 64 bit G_LOAD/G_STORE.

In PPCRegisterBankInfo.cpp, some codes are from AArch64 target. Since PPC Global ISel is still in an early stage, I just made another copy in PPC instead of putting the sharing codes to a common place. Will do the refactor later.

[tschuett]

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

145

Did you say 64bit?

[arsenm]

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

361–362

What's the problem with importing the tablegen patterns? This looks pretty simple?

376

Shouldn't create one off MachineIRBuilders

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

144–145

This cannot happen

161–163

Should put in utils

176

I wouldn't really call FNEG or FABS FP opcodes

[shchenz]

address comments

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

145

Sure, I can remove this. There is no user for this now.

361–362

PPC does not have a single rule for the simple pattern below in td files, it uses one rule to match all patterns in the PPCTargetLowering.

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

161–163

I will post another NFC patch to refactor these functions.

176

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?

[arsenm]

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

123

I think the intent is the call here shouldn't need to look at uses

143

This call doesn't make any sense, you're just getting a pointer to MI with more steps

176

They don't really have floating point semantics. They're non-canonicalizing bit operations, but maybe it makes sense for PPC instructions?

[shchenz]

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

123

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

143

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?

176

Yes, we have floating point abs/neg instrutions, fabs and fneg on PPC.

[arsenm]

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

123

I guess I thought the point of all the mapping costs were to let the pass look at the uses. I haven't looked that closely at this usage of regbankselect

143

Operand 0 is the result operand. Operand 1 is the stored value

[shchenz]

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

143

hmm, I dump the operands for the MI G_STORE %1:_(s64), %0:_(p0) :: (store (s64) into %ir.p), the operand 0 is %1:_(s64), operand 1 is %0:_(p0).

the ll is:

define void @store_i64(i64* %p) {
entry:
  store i64 100, i64* %p, align 8
  ret void
}

MIR before reg bank selection is:

# Machine code for function store_i64: IsSSA, TracksLiveness, Legalized
Function Live Ins: $x3

bb.1.entry:
  liveins: $x3
  %0:_(p0) = COPY $x3
  %1:_(s64) = G_CONSTANT i64 100
  G_STORE %1:_(s64), %0:_(p0) :: (store (s64) into %ir.p)
  BLR8 implicit $lr8, implicit $rm

# End machine code for function store_i64.

[arsenm]

I'd like to see more test coverage of the various use scans for regbankselect. In particular multiple uses, and mixed type uses between FP and integer

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

123

The tests aren't covering these various use checks

143

Sorry, I thought I was looking at load

llvm/test/CodeGen/PowerPC/GlobalISel/load-store-64bit.ll

5

New tests should use opaque pointers

[shchenz]

address @arsenm comments

[shchenz]

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

123

Sorry, I meant to add more cases, but seems now there are not too many generic opcodes support on PPC target for globalisel, for example G_FPTOSI/G_SITOFP...

[arsenm]

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

373–375

The verifier will catch this with correctly defined legalizer rules

388

Don't bother with this assert, the verifier checks this

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

123

Should add a few simple ones in a pre-commit to use in tests here

[shchenz]

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

123

pre-commit cases will crash because there is no load/store support for PPC GISEL . Do you mean I pre-commit these crash ones guarded with not --crash?

[shchenz]

address comments

[arsenm]

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

123

No, I mean do a simple patch to add support for a few more FP operations. You have working arg and return, so it's not dependent on load/store first

[shchenz]

Make load/store selection be parent of constantFP selection patch

[amyk]

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

133

Minor nit: Capitalize the first word of the variable to adhere to the LLVM coding guidelines.

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

121

nit: Maybe we can say Unsupported load type!

But also a question - I know there was a comment earlier stating that the assert in the instruction selector wasn't needed because the verifier checks that kind of stuff, but is this the same for the register bank selection pass?

[arsenm]

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

121

Yes, there's an assert for legal operations before RegBankSelect

[shchenz]

address comments

[shchenz]

add more cases

[arsenm]

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

47

This is missing the pointer operand. I'm surprised this doesn't fail anywhere

[shchenz]

use all operands for legalizing G_LOAD/G_STORE

[shchenz]

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

47

Good catch. The previous version only matches the first operand. And our cases are all have valid first operand and also the other operands.
I tried to add some MIR cases that pass with previous version but failed with the new update version, for example, making the second operand as scalar type instead of pointer type or making the MI have no memory operand, but both failed. Seems G_LOAD/G_STORE has its own constraints, for example, the second operand must be a pointer and there must be memory operand. A 32-bit pointer would be a possible case, but that is not the case PPC currently trying to support, for now, only PPC64LE is supported.

[arsenm]

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

121

The tests don't involve different use lists that would fail this check

[shchenz]

add test cases with more than one use for the load

[shchenz]

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

121

updated.

[arsenm]

LGTM with nit

llvm/test/CodeGen/PowerPC/GlobalISel/load-store-64bit.ll

89

Use named values in tests

[shchenz]

llvm/test/CodeGen/PowerPC/GlobalISel/load-store-64bit.ll

89

Sure, will change this in commit.

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

 //===----------------------------------------------------------------------===//
 
 #include "PPC.h"
 #include "PPCInstrInfo.h"
 #include "PPCMachineFunctionInfo.h"
 #include "PPCRegisterBankInfo.h"
 #include "PPCSubtarget.h"
 #include "PPCTargetMachine.h"
+#include "llvm/CodeGen/GlobalISel/GenericMachineInstrs.h"
 #include "llvm/CodeGen/GlobalISel/InstructionSelector.h"
 #include "llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h"
+#include "llvm/CodeGen/GlobalISel/MachineIRBuilder.h"
 #include "llvm/CodeGen/MachineConstantPool.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/IR/IntrinsicsPowerPC.h"
 #include "llvm/Support/Debug.h"
 
 #define DEBUG_TYPE "ppc-gisel"
 
 using namespace llvm;
if (!RBI.constrainGenericRegister(DstReg, *DstRC, MRI)) {
     LLVM_DEBUG(dbgs() << "Failed to constrain " << TII.getName(I.getOpcode())
                       << " operand\n");
     return false;
   }
 
   return true;
 }
 
+static unsigned selectLoadStoreOp(unsigned GenericOpc, unsigned RegBankID,
+                                  unsigned OpSize) {
+  const bool isStore = GenericOpc == TargetOpcode::G_STORE;
+  switch (RegBankID) {
+  case PPC::GPRRegBankID:
+    switch (OpSize) {
+    case 64:
+      return isStore ? PPC::STD : PPC::LD;
+    default:
+      llvm_unreachable("Unexpected size!");
+    }
+    break;
+  case PPC::FPRRegBankID:
+    switch (OpSize) {
+    case 64:
+      return isStore ? PPC::STFD : PPC::LFD;
+    default:
+      llvm_unreachable("Unexpected size!");
+    }
+    break;
+  default:
+    llvm_unreachable("Unexpected register bank!");
+  }
+  return GenericOpc;
+}
+
 bool PPCInstructionSelector::materializeFP(MachineInstr &I,
                                            MachineRegisterInfo &MRI,
                                            MachineFunction &MF) const {
   const DebugLoc &DbgLoc = I.getDebugLoc();
 
   // TODO: handle 32-bit.
   assert(STI.isPPC64() && "Only 64-bit is supported!");
 
bool PPCInstructionSelector::select(MachineInstr &I) {
 
   if (selectImpl(I, *CoverageInfo))
     return true;
 
   unsigned Opcode = I.getOpcode();
 
   switch (Opcode) {
   default:
     return false;
+  case TargetOpcode::G_LOAD:
+  case TargetOpcode::G_STORE: {
+    GLoadStore &LdSt = cast<GLoadStore>(I);
+    LLT PtrTy = MRI.getType(LdSt.getPointerReg());
+
+    if (PtrTy != LLT::pointer(0, 64)) {
+      LLVM_DEBUG(dbgs() << "Load/Store pointer has type: " << PtrTy
+                        << ", expected: " << LLT::pointer(0, 64) << '\n');
+      return false;
+    }
+
+    assert((LdSt.getMemSizeInBits() ==
+            MRI.getType(LdSt.getReg(0)).getSizeInBits()) &&
+           "load/store size is not same with the defs!");
+
+    auto SelectLoadStoreAddressingMode = [&]() -> MachineInstr * {
+      const unsigned NewOpc = selectLoadStoreOp(
+          I.getOpcode(), RBI.getRegBank(LdSt.getReg(0), MRI, TRI)->getID(),
+          LdSt.getMemSizeInBits());
+
+      if (NewOpc == I.getOpcode())
+        return nullptr;
+
+      // For now, simply use DForm with load/store addr as base and 0 as imm.
+      // FIXME: optimize load/store with some specific address patterns.
+      I.setDesc(TII.get(NewOpc));
+      assert(I.getOperand(1).isReg() && "load/store address is not a reg!");
+      Register AddrReg = I.getOperand(1).getReg();
+      bool IsKill = I.getOperand(1).isKill();
+      I.getOperand(1).ChangeToImmediate(0);
+      I.addOperand(*I.getParent()->getParent(),
+                   MachineOperand::CreateReg(AddrReg, /* isDef */ false,
+                                             /* isImp */ false, IsKill));
+      return &I;
+    };
+
+    MachineInstr *LoadStore = SelectLoadStoreAddressingMode();
+    if (!LoadStore)
+      return false;
+
+    return constrainSelectedInstRegOperands(*LoadStore, TII, TRI, RBI);
+  }
   case TargetOpcode::G_FCONSTANT:
     return materializeFP(I, MRI, MF);
   case TargetOpcode::G_CONSTANT:
     return selectConst(I, MBB, MRI);
   case TargetOpcode::G_SEXT_INREG:
     return selectSExt(I, MBB, MRI);
   }
   return false;

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

getActionDefinitionsBuilder(G_SEXT_INREG)
        .legalForTypeWithAnyImm({S64});
   getActionDefinitionsBuilder({G_AND, G_OR, G_XOR})
       .legalFor({S64})
       .clampScalar(0, S64, S64);
 
   getActionDefinitionsBuilder({G_FADD, G_FSUB, G_FMUL, G_FDIV, G_FCONSTANT})
       .legalFor({S32, S64});
 
+  // For now only handle 64 bit, we only support 64 bit integer and zext/sext is
+  // not ready.
+  getActionDefinitionsBuilder({G_LOAD, G_STORE})
+      .legalFor({S64});
+
   getLegacyLegalizerInfo().computeTables();
 }

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

public:
 
   const RegisterBank &getRegBankFromRegClass(const TargetRegisterClass &RC,
                                              LLT Ty) const override;
   const InstructionMapping &
   getInstrMapping(const MachineInstr &MI) const override;
 
   InstructionMappings
   getInstrAlternativeMappings(const MachineInstr &MI) const override;
+
+private:
+  /// Maximum recursion depth for hasFPConstraints.
+  const unsigned MaxFPRSearchDepth = 2;
+
+  /// \returns true if \p MI only uses and defines FPRs.
+  bool hasFPConstraints(const MachineInstr &MI, const MachineRegisterInfo &MRI,
+                        const TargetRegisterInfo &TRI,
+                        unsigned Depth = 0) const;
+
+  /// \returns true if \p MI only uses FPRs.
+  bool onlyUsesFP(const MachineInstr &MI, const MachineRegisterInfo &MRI,
+                  const TargetRegisterInfo &TRI, unsigned Depth = 0) const;
+
+  /// \returns true if \p MI only defines FPRs.
+  bool onlyDefinesFP(const MachineInstr &MI, const MachineRegisterInfo &MRI,
+                     const TargetRegisterInfo &TRI, unsigned Depth = 0) const;
 };
 } // namespace llvm
 
 #endif

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

case TargetOpcode::G_FCONSTANT: {
     LLT Ty = MRI.getType(MI.getOperand(0).getReg());
     unsigned Size = Ty.getSizeInBits();
 
     assert((Size == 32 || Size == 64) && "Unsupported floating point types!\n");
     OperandsMapping = getOperandsMapping(
         {getValueMapping(Size == 32 ? PMI_FPR32 : PMI_FPR64), nullptr});
     break;
   }
+  case TargetOpcode::G_LOAD: {
+    assert(MRI.getType(MI.getOperand(0).getReg()).getSizeInBits() == 64 &&
+           "Unsupported types!\n");
+    // Check if that load feeds fp instructions.
+    if (any_of(MRI.use_nodbg_instructions(MI.getOperand(0).getReg()),
+               [&](const MachineInstr &UseMI) {
+                 // If we have at least one direct use in a FP instruction,
+                 // assume this was a floating point load in the IR. If it was
+                 // not, we would have had a bitcast before reaching that
+                 // instruction.
+                 //
+                 // Int->FP conversion operations are also captured in
+                 // onlyDefinesFP().
+                 return onlyUsesFP(UseMI, MRI, TRI);
+               }))
+      OperandsMapping = getOperandsMapping(
+          {getValueMapping(PMI_FPR64), getValueMapping(PMI_GPR64)});
+    else
+      OperandsMapping = getOperandsMapping(
+          {getValueMapping(PMI_GPR64), getValueMapping(PMI_GPR64)});
+    break;
+  }
+  case TargetOpcode::G_STORE: {
+    // Check if the store is fed by fp instructions.
+    MachineInstr *DefMI = MRI.getVRegDef(MI.getOperand(0).getReg());
+    if (onlyDefinesFP(*DefMI, MRI, TRI))
+      OperandsMapping = getOperandsMapping(
+          {getValueMapping(PMI_FPR64), getValueMapping(PMI_GPR64)});
+    else
+      OperandsMapping = getOperandsMapping(
+          {getValueMapping(PMI_GPR64), getValueMapping(PMI_GPR64)});
+    break;
+  }
   default:
     return getInvalidInstructionMapping();
   }
 
   return getInstructionMapping(MappingID, Cost, OperandsMapping, NumOperands);
 }
 
+/// Returns whether opcode \p Opc is a pre-isel generic floating-point opcode,
+/// having only floating-point operands.
+/// FIXME: this is copied from target AArch64. Needs some code refactor here to
+/// put this function in GlobalISel/Utils.cpp.
+static bool isPreISelGenericFloatingPointOpcode(unsigned Opc) {
+  switch (Opc) {
+  case TargetOpcode::G_FADD:
+  case TargetOpcode::G_FSUB:
+  case TargetOpcode::G_FMUL:
+  case TargetOpcode::G_FMA:
+  case TargetOpcode::G_FDIV:
+  case TargetOpcode::G_FCONSTANT:
+  case TargetOpcode::G_FPEXT:
+  case TargetOpcode::G_FPTRUNC:
+  case TargetOpcode::G_FCEIL:
+  case TargetOpcode::G_FFLOOR:
+  case TargetOpcode::G_FNEARBYINT:
+  case TargetOpcode::G_FNEG:
+  case TargetOpcode::G_FCOS:
+  case TargetOpcode::G_FSIN:
+  case TargetOpcode::G_FLOG10:
+  case TargetOpcode::G_FLOG:
+  case TargetOpcode::G_FLOG2:
+  case TargetOpcode::G_FSQRT:
+  case TargetOpcode::G_FABS:
+  case TargetOpcode::G_FEXP:
+  case TargetOpcode::G_FRINT:
+  case TargetOpcode::G_INTRINSIC_TRUNC:
+  case TargetOpcode::G_INTRINSIC_ROUND:
+  case TargetOpcode::G_FMAXNUM:
+  case TargetOpcode::G_FMINNUM:
+  case TargetOpcode::G_FMAXIMUM:
+  case TargetOpcode::G_FMINIMUM:
+    return true;
+  }
+  return false;
+}
+
+/// \returns true if a given intrinsic \p ID only uses and defines FPRs.
+static bool isFPIntrinsic(unsigned ID) {
+  // TODO: Add more intrinsics.
+  switch (ID) {
+  default:
+    return false;
+  }
+}
+
+/// FIXME: this is copied from target AArch64. Needs some code refactor here to
+/// put this function in class RegisterBankInfo.
+bool PPCRegisterBankInfo::hasFPConstraints(const MachineInstr &MI,
+                                           const MachineRegisterInfo &MRI,
+                                           const TargetRegisterInfo &TRI,
+                                           unsigned Depth) const {
+  unsigned Op = MI.getOpcode();
+  if (Op == TargetOpcode::G_INTRINSIC && isFPIntrinsic(MI.getIntrinsicID()))
+    return true;
+
+  // Do we have an explicit floating point instruction?
+  if (isPreISelGenericFloatingPointOpcode(Op))
+    return true;
+
+  // No. Check if we have a copy-like instruction. If we do, then we could
+  // still be fed by floating point instructions.
+  if (Op != TargetOpcode::COPY && !MI.isPHI() &&
+      !isPreISelGenericOptimizationHint(Op))
+    return false;
+
+  // Check if we already know the register bank.
+  auto *RB = getRegBank(MI.getOperand(0).getReg(), MRI, TRI);
+  if (RB == &PPC::FPRRegBank)
+    return true;
+  if (RB == &PPC::GPRRegBank)
+    return false;
+
+  // We don't know anything.
+  //
+  // If we have a phi, we may be able to infer that it will be assigned a FPR
+  // based off of its inputs.
+  if (!MI.isPHI() || Depth > MaxFPRSearchDepth)
+    return false;
+
+  return any_of(MI.explicit_uses(), [&](const MachineOperand &Op) {
+    return Op.isReg() &&
+           onlyDefinesFP(*MRI.getVRegDef(Op.getReg()), MRI, TRI, Depth + 1);
+  });
+}
+
+/// FIXME: this is copied from target AArch64. Needs some code refactor here to
+/// put this function in class RegisterBankInfo.
+bool PPCRegisterBankInfo::onlyUsesFP(const MachineInstr &MI,
+                                     const MachineRegisterInfo &MRI,
+                                     const TargetRegisterInfo &TRI,
+                                     unsigned Depth) const {
+  switch (MI.getOpcode()) {
+  case TargetOpcode::G_FPTOSI:
+  case TargetOpcode::G_FPTOUI:
+  case TargetOpcode::G_FCMP:
+  case TargetOpcode::G_LROUND:
+  case TargetOpcode::G_LLROUND:
+    return true;
+  default:
+    break;
+  }
+  return hasFPConstraints(MI, MRI, TRI, Depth);
+}
+
+/// FIXME: this is copied from target AArch64. Needs some code refactor here to
+/// put this function in class RegisterBankInfo.
+bool PPCRegisterBankInfo::onlyDefinesFP(const MachineInstr &MI,
+                                        const MachineRegisterInfo &MRI,
+                                        const TargetRegisterInfo &TRI,
+                                        unsigned Depth) const {
+  switch (MI.getOpcode()) {
+  case TargetOpcode::G_SITOFP:
+  case TargetOpcode::G_UITOFP:
+    return true;
+  default:
+    break;
+  }
+  return hasFPConstraints(MI, MRI, TRI, Depth);
+}
+
 RegisterBankInfo::InstructionMappings
 PPCRegisterBankInfo::getInstrAlternativeMappings(const MachineInstr &MI) const {
   // TODO Implement.
   return RegisterBankInfo::getInstrAlternativeMappings(MI);
 }

llvm/test/CodeGen/PowerPC/GlobalISel/load-store-64bit.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 i64 @load_i64(i64* %p) {
+; CHECK-LABEL: load_i64:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    ld r3, 0(r3)
+; CHECK-NEXT:    blr
+entry:
+  %0 = load i64, i64* %p, align 8
+  ret i64 %0
+}
+
+define double @load_f64(double* %p) {
+; CHECK-LABEL: load_f64:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    lfd f1, 0(r3)
+; CHECK-NEXT:    blr
+entry:
+  %0 = load double, double* %p, align 8
+  ret double %0
+}
+
+define void @store_i64(i64* %p) {
+; CHECK-LABEL: store_i64:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    li r4, 100
+; CHECK-NEXT:    std r4, 0(r3)
+; CHECK-NEXT:    blr
+entry:
+  store i64 100, i64* %p, align 8
+  ret void
+}
+
+define void @store_f64(double* %p) {
+; CHECK-LABEL: store_f64:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    addis r4, r2, .LCPI3_0@toc@ha
+; CHECK-NEXT:    lfd f0, .LCPI3_0@toc@l(r4)
+; CHECK-NEXT:    stfd f0, 0(r3)
+; CHECK-NEXT:    blr
+entry:
+  store double 1.000000e+00, double* %p, align 8
+  ret void
+}