Expand BuildPairF64 by spilling to stack slot and reloading from it

[sstankovic]

Summary

[mips] Expand BuildPairF64 to a spill and reload when the O32 FPXX ABI is enabled and mthc1 and dmtc1 are not available (e.g. on MIPS32r1)

This prevents the upper 32-bits of a double precision value from being moved to the FPU with mtc1 to an odd-numbered FPU register. This is necessary to ensure that the code generated executes correctly regardless of the current FPU mode.

MIPS32r2 and above continues to use mtc1/mthc1, while MIPS-IV and above continue to use dmtc1.

[dsanders]

I think this is fairly close. There are quite a few comments below but they are all simple issues.

The commit message ought to be more informative than it currently is since most readers aren't going to know why this is necessary. Perhaps something like:

[mips] Expand BuildPairF64 to a spill and reload when the O32 FPXX ABI is enabled and mthc1 and dmtc1 are not available (e.g. on MIPS32r1)

This prevents the upper 32-bits of a double precision value from being moved to the FPU with mtc1 to an
odd-numbered FPU register. This is necessary to ensure that the code generated executes correctly
regardless of the current FPU mode.

MIPS32r2 and above continues to use mtc1/mthc1, while MIPS-IV and above continue to use dmtc1.

lib/Target/Mips/MipsSEFrameLowering.cpp

271–274

I've been asked not to repeat the function name in the comments.

...when ABI is fpxx and mthc1 is not available...

It would be good to add a comment mentioning that the case where dmtc1 is available doesn't need to be handled here because it never creates a BuildPair node. It would be great if you could also add this comment to MipsSEInstrInfo::expandBuildPairF64() too.

On the subject of MipsSEInstrInfo::expandBuildPairF64(), it contains a comment about the spill and reload case. You need to update it to say that that case is handled by the frame lowering code. It would be a good idea to add an assertion there too just in case ExpandPseudo::expandBuildPairF64() isn't called for some reason.

282–283

MipsSubTarget::hasMips32r2() is true for MIPS32r6 too so you could drop the explicit check for hasMips32r6(). Likewise for the same code in MipsSEInstrInfo::expandBuildPairF64().

295

Should we hardcode this register class, or pick between AFGR64/FGR64 depending on whether we have 64-bit FPR's?
It should be impossible to have FGR64 on MIPS-II or MIPS32r1 (which are the two cases that trigger this code) but I don't think we have anything enforcing that at the moment.

If we do hardcode the class, we should at least assert(!TM.getSubtarget<MipsSubtarget>().isFP64()) so that failing to prevent -mfp64 on MIPS-II and MIPS32r1 elsewhere will fail loudly here.

297–299

It should be fairly easy to re-use the stack slot. You can store FI in the MipsFunctionInfo which you can access with MF.getInfo<MipsFunctionInfo>

test/CodeGen/Mips/fpxx.ll

2–3

It would be good to test the mtc1/mthc1 (mips32r2) and dmtc1 (mips4 or mips64r1) cases in this test. We cover them elsewhere as a side-effect of testing something else but it would be good to cover them deliberately in this test.

[dsanders]

It just occurred to me that we haven't covered moves from the FPU. They need to do the same thing.

[dsanders]

lib/Target/Mips/MipsSEFrameLowering.cpp

300–301

The 0 and 4 need to be the other way around for big-endian.

[dsanders]

lib/Target/Mips/MipsSEFrameLowering.cpp

300–301

Also, LoReg and HiReg might not be killed in all cases. Query the operand with I->getOperand(1).isKill().

[dsanders]

lib/Target/Mips/MipsSEFrameLowering.cpp

300–301

The 0 and 4 need to be the other way around for big-endian.

It turns out that I'm wrong about this. In big-endian mode, the hi/lo operands have already been swapped so we don't need to account for it in the offset. Sorry for the noise.

[sstankovic]

Patch is updated.

[sstankovic]

lib/Target/Mips/MipsSEFrameLowering.cpp

271–274

It would be good to add a comment mentioning that the case where dmtc1 is available doesn't need to be handled here because it never creates a BuildPair node. It would be great if you could also add this comment to MipsSEInstrInfo::expandBuildPairF64() too.

Done.

On the subject of MipsSEInstrInfo::expandBuildPairF64(), it contains a comment about the spill and reload case. You need to update it to say that that case is handled by the frame lowering code. It would be a good idea to add an assertion there too just in case ExpandPseudo::expandBuildPairF64() isn't called for some reason.

Done

282–283

Done

295

I hardcoded the class and added the assert.

297–299

Done.

300–301

Also, LoReg and HiReg might not be killed in all cases. Query the operand with I->getOperand(1).isKill().

Done

test/CodeGen/Mips/fpxx.ll

2–3

I added the test for mips32r2. What should the mips64r1/mips4 case test?

[sstankovic]

It just occurred to me that we haven't covered moves from the FPU. They need to do the same thing.

Should this be part of this patch or a separate patch?

[dsanders]

In D4465#13, @sstankovic wrote:

It just occurred to me that we haven't covered moves from the FPU. They need to do the same thing.

Should this be part of this patch or a separate patch?

It can be a separate patch. I have an unfinished implementation of it for FP64A in my working copy at the moment. The code will be very similar between FP64A and FPXX and has very similar test cases so I could do both in the same patch if you like.

lib/Target/Mips/MipsMachineFunction.h

144

I see you are using -1 to mean invalid. -1 is normally a valid FrameIndex (negative numbers are used for stack objects with a fixed offset) so we should mention that -1 is used as an invalid value.

Also, please turn this comment into a doxygen comment using '///'.

test/CodeGen/Mips/fpxx.ll

2–3

They should test that the NOFPXX variant produces a dmtc1 and that the FPXX variant reports an error saying that FPXX is not permitted with the N32/N64 ABI's.

It's not strictly necessary, but it would be good to leave a RUN-TODO: line for the O32 (-mattr=-n64,+o32) on MIPS4/MIPS64r1 with and without FPXX cases. We don't support O32 on 64-bit ISA's at the moment but it will be helpful to be able to find all the tests easily (with something like 'grep -r RUN-TODO . | grep o32') when we come to support it.

[dsanders]

... so I could do both in the same patch if you like.

Just fixing some poor phrasing I noticed after clicking send. I mean I could add the FPXX code to handle move from FPU at the same time as I add the code to handle FP64A move to/from FPU.

[sstankovic]

Patch is updated.

[sstankovic]

It can be a separate patch. I have an unfinished implementation of it for FP64A in my working copy at the moment. The code will be very similar between FP64A and FPXX and has very similar test cases so I could do both in the same patch if you like.

OK.

lib/Target/Mips/MipsMachineFunction.h

144

Done.

test/CodeGen/Mips/fpxx.ll

2–3

I added a new test for dtmc1. Existing tests when compiled for n64 receive double arguments in float registers, so dmtc1 is not used.

I also added RUN-TODO lines.

[dsanders]

LGTM with one last change to match the function labels in all the RUN lines of the testcase.

As discussed, I'll cover the move-from-fpu case for FPXX in my FP64A patch.

test/CodeGen/Mips/fpxx.ll

24

You can make this apply to all the tests using multiple FileCheck prefixes (e.g. 'FileCheck %s -check-prefix=ALL -check-prefix=32-NOFPXX' and using 'ALL-LABEL:').

Likewise for the others below

[sstankovic]

I added -check-prefix=ALL to all RUN lines except two "RUN: not" lines that check for error messages (otherwise test won't execute). I also moved checks for error messages to the start of the test file.

[dsanders]

LGTM

[sstankovic]

Closed by commit rL212930 (authored by @sstankovic).

lib/Target/Mips/MipsMachineFunction.h

 };
 
 /// MipsFunctionInfo - This class is derived from MachineFunction private
 /// Mips target-specific information for each MachineFunction.
 class MipsFunctionInfo : public MachineFunctionInfo {
 public:
   MipsFunctionInfo(MachineFunction &MF)
       : MF(MF), SRetReturnReg(0), GlobalBaseReg(0), Mips16SPAliasReg(0),
-        VarArgsFrameIndex(0), CallsEhReturn(false), SaveS2(false) {}
+        VarArgsFrameIndex(0), CallsEhReturn(false), SaveS2(false),
+        BuildPairF64_FI(-1) {}
 
   ~MipsFunctionInfo();
 
   unsigned getSRetReturnReg() const { return SRetReturnReg; }
   void setSRetReturnReg(unsigned Reg) { SRetReturnReg = Reg; }
 
   bool globalBaseRegSet() const;
   unsigned getGlobalBaseReg();
public:
 
   /// \brief Create a MachinePointerInfo that has a MipsCallEntr object
   /// representing a GOT entry for a global function.
   MachinePointerInfo callPtrInfo(const GlobalValue *Val);
 
   void setSaveS2() { SaveS2 = true; }
   bool hasSaveS2() const { return SaveS2; }
 
+  int getBuildPairF64_FI(const TargetRegisterClass *RC);
+
   std::map<const char *, const llvm::Mips16HardFloatInfo::FuncSignature *>
   StubsNeeded;
 
 private:
   virtual void anchor();
 
   MachineFunction& MF;
   /// SRetReturnReg - Some subtargets require that sret lowering includes
private:
   bool CallsEhReturn;
 
   /// Frame objects for spilling eh data registers.
   int EhDataRegFI[4];
 
   // saveS2
   bool SaveS2;
 
+  // FrameIndex for expanding BuildPairF64 nodes to spill and reload when the
+  // O32 FPXX ABI is enabled.
+  int BuildPairF64_FI;
+
   /// MipsCallEntry maps.
   StringMap<const MipsCallEntry *> ExternalCallEntries;
   ValueMap<const GlobalValue *, const MipsCallEntry *> GlobalCallEntries;
 };
 
 } // end of namespace llvm
 
 #endif // MIPS_MACHINE_FUNCTION_INFO_H

lib/Target/Mips/MipsMachineFunction.cpp

MachinePointerInfo MipsFunctionInfo::callPtrInfo(const GlobalValue *Val) {
   const MipsCallEntry *&E = GlobalCallEntries[Val];
 
   if (!E)
     E = new MipsCallEntry(Val);
 
   return MachinePointerInfo(E);
 }
 
+int MipsFunctionInfo::getBuildPairF64_FI(const TargetRegisterClass *RC) {
+  if (BuildPairF64_FI == -1) {
+    BuildPairF64_FI = MF.getFrameInfo()->CreateStackObject(RC->getSize(),
+        RC->getAlignment(), false);
+  }
+  return BuildPairF64_FI;
+}
+
 void MipsFunctionInfo::anchor() { }

lib/Target/Mips/MipsSEFrameLowering.cpp

private:
   void expandLoadCCond(MachineBasicBlock &MBB, Iter I);
   void expandStoreCCond(MachineBasicBlock &MBB, Iter I);
   void expandLoadACC(MachineBasicBlock &MBB, Iter I, unsigned RegSize);
   void expandStoreACC(MachineBasicBlock &MBB, Iter I, unsigned MFHiOpc,
                       unsigned MFLoOpc, unsigned RegSize);
   bool expandCopy(MachineBasicBlock &MBB, Iter I);
   bool expandCopyACC(MachineBasicBlock &MBB, Iter I, unsigned MFHiOpc,
                      unsigned MFLoOpc);
+  bool expandBuildPairF64(MachineBasicBlock &MBB,
+                          MachineBasicBlock::iterator I, bool FP64) const;
 
   MachineFunction &MF;
   MachineRegisterInfo &MRI;
 };
 }
 
 ExpandPseudo::ExpandPseudo(MachineFunction &MF_)
   : MF(MF_), MRI(MF.getRegInfo()) {}
case Mips::STORE_ACC64:
     expandStoreACC(MBB, I, Mips::PseudoMFHI, Mips::PseudoMFLO, 4);
     break;
   case Mips::STORE_ACC64DSP:
     expandStoreACC(MBB, I, Mips::MFHI_DSP, Mips::MFLO_DSP, 4);
     break;
   case Mips::STORE_ACC128:
     expandStoreACC(MBB, I, Mips::PseudoMFHI64, Mips::PseudoMFLO64, 8);
     break;
+  case Mips::BuildPairF64:
+    if (expandBuildPairF64(MBB, I, false))
+      MBB.erase(I);
+    return false;
+  case Mips::BuildPairF64_64:
+    if (expandBuildPairF64(MBB, I, true))
+      MBB.erase(I);
+    return false;
   case TargetOpcode::COPY:
     if (!expandCopy(MBB, I))
       return false;
     break;
   default:
     return false;
   }
 
bool ExpandPseudo::expandCopyACC(MachineBasicBlock &MBB, Iter I,
   BuildMI(MBB, I, DL, TII.get(TargetOpcode::COPY), DstLo)
     .addReg(VR0, RegState::Kill);
   BuildMI(MBB, I, DL, TII.get(MFHiOpc), VR1).addReg(Src, SrcKill);
   BuildMI(MBB, I, DL, TII.get(TargetOpcode::COPY), DstHi)
     .addReg(VR1, RegState::Kill);
   return true;
 }
 
+/// This method expands the same instruction that MipsSEInstrInfo::
+/// expandBuildPairF64 does, for the case when ABI is fpxx and mthc1 is
+/// not available. It is implemented here because frame indexes are
+/// eliminated before MipsSEInstrInfo::expandBuildPairF64 is called.
+bool ExpandPseudo::expandBuildPairF64(MachineBasicBlock &MBB,
+                                      MachineBasicBlock::iterator I,
+                                      bool FP64) const {
+  // For fpxx and when mthc1 is not available, use:
+  //   spill + reload via ldc1
+  //
+  // The case where dmtc1 is available doesn't need to be handled here
+  // because it never creates a BuildPairF64 node.
+
+  const TargetMachine &TM = MF.getTarget();
+  if (TM.getSubtarget<MipsSubtarget>().isABI_FPXX()
+      && !TM.getSubtarget<MipsSubtarget>().hasMTHC1()) {
+    const MipsSEInstrInfo &TII =
+      *static_cast<const MipsSEInstrInfo*>(TM.getInstrInfo());
+    const MipsRegisterInfo &TRI =
+      *static_cast<const MipsRegisterInfo*>(TM.getRegisterInfo());
+
+    unsigned DstReg = I->getOperand(0).getReg();
+    unsigned LoReg = I->getOperand(1).getReg();
+    unsigned HiReg = I->getOperand(2).getReg();
+
+    // It should be impossible to have FGR64 on MIPS-II or MIPS32r1 (which are
+    // the cases where mthc1 is not available).
+    assert(!TM.getSubtarget<MipsSubtarget>().isFP64bit());
+
+    const TargetRegisterClass *RC = &Mips::GPR32RegClass;
+    const TargetRegisterClass *RC2 = &Mips::AFGR64RegClass;
+
+    int FI = MF.getInfo<MipsFunctionInfo>()->getBuildPairF64_FI(RC2);
+    TII.storeRegToStack(MBB, I, LoReg, I->getOperand(1).isKill(), FI, RC, &TRI,
+                        0);
+    TII.storeRegToStack(MBB, I, HiReg, I->getOperand(2).isKill(), FI, RC, &TRI,
+                        4);
+    TII.loadRegFromStack(MBB, I, DstReg, FI, RC2, &TRI, 0);
+    return true;
+  }
+
+  return false;
+}
+
 MipsSEFrameLowering::MipsSEFrameLowering(const MipsSubtarget &STI)
     : MipsFrameLowering(STI, STI.stackAlignment()) {}
 
 unsigned MipsSEFrameLowering::ehDataReg(unsigned I) const {
   static const unsigned EhDataReg[] = {
     Mips::A0, Mips::A1, Mips::A2, Mips::A3
   };
   static const unsigned EhDataReg64[] = {

lib/Target/Mips/MipsSEInstrInfo.cpp

 void MipsSEInstrInfo::expandBuildPairF64(MachineBasicBlock &MBB,
                                          MachineBasicBlock::iterator I,
                                          bool FP64) const {
   unsigned DstReg = I->getOperand(0).getReg();
   unsigned LoReg = I->getOperand(1).getReg(), HiReg = I->getOperand(2).getReg();
   const MCInstrDesc& Mtc1Tdd = get(Mips::MTC1);
   DebugLoc dl = I->getDebugLoc();
   const TargetRegisterInfo &TRI = getRegisterInfo();
-  bool HasMTHC1 = TM.getSubtarget<MipsSubtarget>().hasMips32r2() ||
-                  TM.getSubtarget<MipsSubtarget>().hasMips32r6();
 
   // When mthc1 is available, use:
   //   mtc1 Lo, $fp
   //   mthc1 Hi, $fp
   //
-  // Otherwise, for FP64:
+  // Otherwise, for O32 FPXX ABI:
   //   spill + reload via ldc1
-  // This has not been implemented since FP64 on MIPS32 and earlier is not
-  // supported.
+  // This case is handled by the frame lowering code.
   //
   // Otherwise, for FP32:
   //   mtc1 Lo, $fp
   //   mtc1 Hi, $fp + 1
+  //
+  // The case where dmtc1 is available doesn't need to be handled here
+  // because it never creates a BuildPairF64 node.
 
   BuildMI(MBB, I, dl, Mtc1Tdd, TRI.getSubReg(DstReg, Mips::sub_lo))
     .addReg(LoReg);
 
-  if (HasMTHC1 || FP64) {
-    assert(TM.getSubtarget<MipsSubtarget>().hasMips32r2() &&
-           "MTHC1 requires MIPS32r2");
-
+  if (TM.getSubtarget<MipsSubtarget>().hasMTHC1()) {
     // FIXME: The .addReg(DstReg) is a white lie used to temporarily work
     //        around a widespread bug in the -mfp64 support.
     //        The problem is that none of the 32-bit fpu ops mention the fact
     //        that they clobber the upper 32-bits of the 64-bit FPR. Fixing that
     //        requires a major overhaul of the FPU implementation which can't
     //        be done right now due to time constraints.
     //        MTHC1 is one of two instructions that are affected since they are
     //        the only instructions that don't read the lower 32-bits.
     //        We therefore pretend that it reads the bottom 32-bits to
     //        artificially create a dependency and prevent the scheduler
     //        changing the behaviour of the code.
     BuildMI(MBB, I, dl, get(FP64 ? Mips::MTHC1_D64 : Mips::MTHC1_D32), DstReg)
         .addReg(DstReg)
         .addReg(HiReg);
-  } else
+  } else if (TM.getSubtarget<MipsSubtarget>().isABI_FPXX())
+    llvm_unreachable("BuildPairF64 not expanded in frame lowering code!");
+  else
     BuildMI(MBB, I, dl, Mtc1Tdd, TRI.getSubReg(DstReg, Mips::sub_hi))
       .addReg(HiReg);
 }
 
 void MipsSEInstrInfo::expandEhReturn(MachineBasicBlock &MBB,
                                      MachineBasicBlock::iterator I) const {
   // This pseudo instruction is generated as part of the lowering of
   // ISD::EH_RETURN. We convert it to a stack increment by OffsetReg, and

lib/Target/Mips/MipsSubtarget.h

bool enablePostRAScheduler(CodeGenOpt::Level OptLevel,
                              AntiDepBreakMode& Mode,
                              RegClassVector& CriticalPathRCs) const override;
 
   /// Only O32 and EABI supported right now.
   bool isABI_EABI() const { return MipsABI == EABI; }
   bool isABI_N64() const { return MipsABI == N64; }
   bool isABI_N32() const { return MipsABI == N32; }
   bool isABI_O32() const { return MipsABI == O32; }
-  bool isABI_FPXX() const { return false; } // TODO: add check for FPXX
+  bool isABI_FPXX() const { return isABI_O32() && IsFPXX; }
   unsigned getTargetABI() const { return MipsABI; }
 
   /// This constructor initializes the data members to match that
   /// of the specified triple.
   MipsSubtarget(const std::string &TT, const std::string &CPU,
                 const std::string &FS, bool little, Reloc::Model RM,
                 MipsTargetMachine *TM);
 
public:
   bool mipsSEUsesSoftFloat() const;
 
   bool enableLongBranchPass() const {
     return hasStandardEncoding() || allowMixed16_32();
   }
 
   /// Features related to the presence of specific instructions.
   bool hasExtractInsert() const { return !inMips16Mode() && hasMips32r2(); }
+  bool hasMTHC1() const { return hasMips32r2(); }
 
   const InstrItineraryData &getInstrItineraryData() const { return InstrItins; }
   bool allowMixed16_32() const { return inMips16ModeDefault() |
                                         AllowMixed16_32;}
 
   bool os16() const { return Os16;};
 
   bool isTargetNaCl() const { return TargetTriple.isOSNaCl(); }

test/CodeGen/Mips/abiflags-xx.ll

 ; RUN: llc -filetype=asm -mtriple mipsel-unknown-linux -mcpu=mips32 -mattr=fpxx %s -o - | FileCheck %s
-; XFAIL: *
 
 ; CHECK: .nan    legacy
 ; CHECK: .module fp=xx
 

test/CodeGen/Mips/fpxx.ll

+; RUN: llc -march=mipsel -mcpu=mips32 < %s | FileCheck %s -check-prefix=32-NOFPXX
+; RUN: llc -march=mipsel -mcpu=mips32 -mattr=fpxx < %s | FileCheck %s -check-prefix=32-FPXX
+
+; RUN: llc -march=mipsel -mcpu=mips32r2 < %s | FileCheck %s -check-prefix=32R2-NOFPXX
+; RUN: llc -march=mipsel -mcpu=mips32r2 -mattr=fpxx < %s | FileCheck %s -check-prefix=32R2-FPXX
+
+
+define double @call1(double %d, ...) {
+  ret double %d
+
+; 32-NOFPXX:    mtc1    $4, $f0
+; 32-NOFPXX:    mtc1    $5, $f1
+
+; 32-FPXX:       addiu   $sp, $sp, -8
+; 32-FPXX:       sw      $4, 0($sp)
+; 32-FPXX:       sw      $5, 4($sp)
+; 32-FPXX:       ldc1    $f0, 0($sp)
+
+; 32R2-NOFPXX:    mtc1    $4, $f0
+; 32R2-NOFPXX:    mthc1   $5, $f0
+
+; 32R2-FPXX:    mtc1    $4, $f0
+; 32R2-FPXX:    mthc1   $5, $f0
+}
+
+define double @call2(i32 %i, double %d) {
+  ret double %d
+
+; 32-NOFPXX:    mtc1    $6, $f0
+; 32-NOFPXX:    mtc1    $7, $f1
+
+; 32-FPXX:       addiu   $sp, $sp, -8
+; 32-FPXX:       sw      $6, 0($sp)
+; 32-FPXX:       sw      $7, 4($sp)
+; 32-FPXX:       ldc1    $f0, 0($sp)
+
+; 32R2-NOFPXX:    mtc1    $6, $f0
+; 32R2-NOFPXX:    mthc1   $7, $f0
+
+; 32R2-FPXX:    mtc1    $6, $f0
+; 32R2-FPXX:    mthc1   $7, $f0
+}
+
+define double @call3(float %f1, float %f2, double %d) {
+  ret double %d
+
+; 32-NOFPXX:    mtc1    $6, $f0
+; 32-NOFPXX:    mtc1    $7, $f1
+
+; 32-FPXX:       addiu   $sp, $sp, -8
+; 32-FPXX:       sw      $6, 0($sp)
+; 32-FPXX:       sw      $7, 4($sp)
+; 32-FPXX:       ldc1    $f0, 0($sp)
+
+; 32R2-NOFPXX:    mtc1    $6, $f0
+; 32R2-NOFPXX:    mthc1   $7, $f0
+
+; 32R2-FPXX:    mtc1    $6, $f0
+; 32R2-FPXX:    mthc1   $7, $f0
+}
+
+define double @call4(float %f, double %d, ...) {
+  ret double %d
+
+; 32-NOFPXX:    mtc1    $6, $f0
+; 32-NOFPXX:    mtc1    $7, $f1
+
+; 32-FPXX:       addiu   $sp, $sp, -8
+; 32-FPXX:       sw      $6, 0($sp)
+; 32-FPXX:       sw      $7, 4($sp)
+; 32-FPXX:       ldc1    $f0, 0($sp)
+
+; 32R2-NOFPXX:    mtc1    $6, $f0
+; 32R2-NOFPXX:    mthc1   $7, $f0
+
+; 32R2-FPXX:    mtc1    $6, $f0
+; 32R2-FPXX:    mthc1   $7, $f0
+}