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

1–2

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

1–2

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 ↗

(On Diff #11324)

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

1–2

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 ↗

(On Diff #11324)

Done.

test/CodeGen/Mips/fpxx.ll

1–2

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/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;
 }
 
+/// expandBuildPairF64 - 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
+  //
+  const TargetMachine &TM = MBB.getParent()->getTarget();
+  bool HasMTHC1 = TM.getSubtarget<MipsSubtarget>().hasMips32r2() ||
+                  TM.getSubtarget<MipsSubtarget>().hasMips32r6();
+
+  if (TM.getSubtarget<MipsSubtarget>().isABI_FPXX() && !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();
+    const TargetRegisterClass *RC = &Mips::GPR32RegClass;
+    const TargetRegisterClass *RC2 = &Mips::AFGR64RegClass;
+
+    // TODO: Reuse the stack slot.
+    int FI = MF.getFrameInfo()->CreateStackObject(RC2->getSize(),
+                                                  RC2->getAlignment(), false);
+    TII.storeRegToStack(MBB, I, LoReg, true, FI, RC, &TRI, 0);
+    TII.storeRegToStack(MBB, I, HiReg, true, 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/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);
 

test/CodeGen/Mips/fpxx.ll

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