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

[Sasa Stankovic]

================
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,
----------------
Daniel Sanders wrote:
> 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.
> 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

================
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();
+
----------------
Daniel Sanders wrote:
> MipsSubTarget::hasMips32r2() is true for MIPS32r6 too so you could drop the explicit check for hasMips32r6(). Likewise for the same code in MipsSEInstrInfo::expandBuildPairF64().
Done

================
Comment at: lib/Target/Mips/MipsSEFrameLowering.cpp:295
@@ +294,3 @@
+    const TargetRegisterClass *RC = &Mips::GPR32RegClass;
+    const TargetRegisterClass *RC2 = &Mips::AFGR64RegClass;
+
----------------
Daniel Sanders wrote:
> 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.
I hardcoded the class and added the assert.

================
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);
----------------
Daniel Sanders wrote:
> 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>
Done.

================
Comment at: lib/Target/Mips/MipsSEFrameLowering.cpp:300-301
@@ +299,4 @@
+                                                  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);
----------------
Daniel Sanders wrote:
> Daniel Sanders wrote:
> > Daniel Sanders wrote:
> > > The 0 and 4 need to be the other way around for big-endian.
> > Also, LoReg and HiReg might not be killed in all cases. Query the operand with I->getOperand(1).isKill().
> > 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.
> Also, LoReg and HiReg might not be killed in all cases. Query the operand with I->getOperand(1).isKill().

Done

================
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
+
----------------
Daniel Sanders wrote:
> 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.
I added the test for mips32r2. What should the mips64r1/mips4 case test?

http://reviews.llvm.org/D4465