[PATCH] Expand BuildPairF64 by spilling to stack slot and reloading from it

[Daniel Sanders]

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.

================
Comment at: lib/Target/Mips/MipsSEFrameLowering.cpp:271-274
@@ -260,1 +270,6 @@
 
+/// 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,
----------------
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.

================
Comment at: lib/Target/Mips/MipsSEFrameLowering.cpp:282-283
@@ +281,4 @@
+  const TargetMachine &TM = MBB.getParent()->getTarget();
+  bool HasMTHC1 = TM.getSubtarget<MipsSubtarget>().hasMips32r2() ||
+                  TM.getSubtarget<MipsSubtarget>().hasMips32r6();
+
----------------
MipsSubTarget::hasMips32r2() is true for MIPS32r6 too so you could drop the explicit check for hasMips32r6(). Likewise for the same code in MipsSEInstrInfo::expandBuildPairF64().

================
Comment at: lib/Target/Mips/MipsSEFrameLowering.cpp:295
@@ +294,3 @@
+    const TargetRegisterClass *RC = &Mips::GPR32RegClass;
+    const TargetRegisterClass *RC2 = &Mips::AFGR64RegClass;
+
----------------
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.

================
Comment at: lib/Target/Mips/MipsSEFrameLowering.cpp:297-299
@@ +296,5 @@
+
+    // 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);
----------------
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>

================
Comment at: test/CodeGen/Mips/fpxx.ll:1-2
@@ +1,3 @@
+; 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
+
----------------
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.

http://reviews.llvm.org/D4465