[PATCH] D21138: [mips] Enable tail calls by default

[Daniel Sanders via llvm-commits]

dsanders requested changes to this revision.
dsanders added a comment.
This revision now requires changes to proceed.

Most of my comments are nits. The important one is the accidental removal of predicates on the new instructions with a 'let Predicates = ...' statement.


================
Comment at: lib/Target/Mips/MicroMipsInstrInfo.td:490
@@ +489,3 @@
+  class TailCallReg_MM<RegisterOperand RO, Instruction JRInst,
+                    RegisterOperand ResRO = RO> :
+    PseudoSE<(outs), (ins RO:$rs), [(MipsTailCall RO:$rs)], II_JR>,
----------------
indentation

================
Comment at: lib/Target/Mips/MicroMipsInstrInfo.td:993-995
@@ -980,2 +992,5 @@
   def PREFX_MM : PrefetchIndexed<"prefx">, POOL32F_PREFX_FM_MM<0x15, 0x1A0>;
+  def TAILCALL_MM : TailCall_MM<J_MM>, ISA_MIPS1_NOT_32R6_64R6;
+  def TAILCALLREG_MM : TailCallReg_MM<GPR32Opnd, JR_MM>,
+                       ISA_MIPS1_NOT_32R6_64R6;
 }
----------------
This doesn't have the predicates you expect because the 'let Predicates = ...' statement discards all the predicates and replaces them with the 1-element list '[InMicroMips]'. The effects of ISA_MIPS1_NOT_32R6_64R6 are lost.

Changing Predicates to AdditionalPredicates should fix this for now but we need to move InMicroMips to another sublist of PredicateControl in the future.

The other instances should be fixed too but that's for another patch

================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:561
@@ -557,1 +560,3 @@
     return Mips::JALRS16_MM;
+   case Mips::TAILCALL_MM:
+    llvm_unreachable("Attempting to shorten the TAILCALL_MM pseudo!");
----------------
Indentation

================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:617-622
@@ -609,1 +616,8 @@
+
+          // TODO: Implement an instruction mapping table of 16bit opcodes to
+          // 32bit opcodes so that an instruction can be expanded. This would
+          // save 16 bits a TAILCALL pseudo is seen as that requires a
+          // fullsized nop. This is becuase b16 has a very short range.
+          // TODO: Permit b16 when branching backwards to the the same function
+          // if it is in range.
           DSI->setDesc(TII->get(getEquivalentCallShort(DSI->getOpcode())));
----------------
becuase -> because

Also, the sentence starting 'This would save 16 bits ...' seems to be missing some words.

================
Comment at: lib/Target/Mips/MipsDelaySlotFiller.cpp:709-713
@@ -694,2 +708,7 @@
     unsigned Opcode = (*Slot).getOpcode();
+    // This is complicated by the tail call optimization. For non-PIC code
+    // there is only a 32bit unconditional branch which can be assumed to
+    // be able to reach the target. b16 only has a ranage of +/- 1 KB.
+    // It's entirely possible that the function is reachable with b16 but
+    // we don't have enough information to make that decision.
     if (InMicroMipsMode && TII->GetInstSizeInBytes(&(*CurrI)) == 2 &&
----------------
I see that 'j' is the tailcall instruction at the moment. I agree that it's fairly safe to rely on this instruction since it has a 256MB range but it's important to mention that this range isn't centered on the instruction like a normal PC-relative instruction. It's technically possible for small offsets like +/-4 to be out of range. JAL has the same issue so I think this is a problem that can be addressed later.

Also, a couple nits:
* Could you clarify that the '32bit' is the opcode size and not the range?
* ranage -> range.

================
Comment at: lib/Target/Mips/MipsSEISelLowering.cpp:30-31
@@ -29,4 +29,4 @@
 static cl::opt<bool>
-EnableMipsTailCalls("enable-mips-tail-calls", cl::Hidden,
-                    cl::desc("MIPS: Enable tail calls."), cl::init(false));
+DisableMipsTailCalls("disable-mips-tail-calls", cl::Hidden,
+                    cl::desc("MIPS: Disable tail calls."), cl::init(false));
 
----------------
I think we should drop the 'enable' and 'disable' (maybe have 'Use' instead) and just have:
  static cl::opt<bool>
  MipsTailCalls("mips-tail-calls", cl::Hidden,
                cl::desc("MIPS: Disable tail calls."), cl::init(true));
That way we control the default with cl::init and override it with -mips-tail-calls=false

================
Comment at: test/CodeGen/Mips/compactbranches/tailrecursion.ll:23
@@ +22,3 @@
+define i32 @f(i32 %a) {
+; CHECK-LABEL: f
+; STATICPRER6:   j
----------------
This matches the first 'f' in the file (e.g. `.file`) rather than the label 'f' at the start of the function. It needs a ':'

================
Comment at: test/CodeGen/Mips/compactbranches/tailrecursion.ll:33
@@ +32,3 @@
+; PICR6:    jrc $25
+; microMIPSR6 does not have delay slots, but uses the menomic 'jr' instead of 'jrc'.
+; PICR6MM:  jr $25
----------------
menomic -> mnemonic

================
Comment at: test/CodeGen/Mips/i64arg.ll:5-25
@@ -4,23 +4,23 @@
 entry:
-; CHECK-DAG: lw $[[R2:[0-9]+]], 80($sp)
-; CHECK-DAG: lw $[[R3:[0-9]+]], 84($sp)
+; CHECK-DAG: lw $[[R2:[0-9]+]], 68($sp)
+; CHECK-DAG: lw $[[R3:[0-9]+]], 64($sp)
 ; CHECK-DAG: move $[[R1:[0-9]+]], $5
 ; CHECK-DAG: move $[[R0:[0-9]+]], $4
 ; CHECK-DAG: ori $6, ${{[0-9]+}}, 3855
 ; CHECK-DAG: ori $7, ${{[0-9]+}}, 22136
 ; CHECK-DAG: lw  $25, %call16(ff1)
-; CHECK: jalr
+; CHECK: jalr $25
   tail call void @ff1(i32 %i, i64 1085102592623924856) nounwind
 ; CHECK-DAG: lw $25, %call16(ff2)
-; CHECK-DAG: move $4, $[[R2]]
-; CHECK-DAG: move $5, $[[R3]]
+; CHECK-DAG: move $4, $[[R3]]
+; CHECK-DAG: move $5, $[[R2]]
 ; CHECK: jalr $25
   tail call void @ff2(i64 %ll, double 3.000000e+00) nounwind
   %sub = add nsw i32 %i, -1
 ; CHECK-DAG: lw $25, %call16(ff3)
-; CHECK-DAG: sw $[[R1]], 28($sp)
-; CHECK-DAG: sw $[[R0]], 24($sp)
-; CHECK-DAG: move $6, $[[R2]]
-; CHECK-DAG: move $7, $[[R3]]
-; CHECK:     jalr $25
+; CHECK-DAG: sw $[[R1]], 76($sp)
+; CHECK-DAG: sw $[[R0]], 72($sp)
+; CHECK-DAG: move $6, $[[R3]]
+; CHECK-DAG: move $7, $[[R2]]
+; CHECK:     jr $25
   tail call void @ff3(i32 %i, i64 %ll, i32 %sub, i64 %ll1) nounwind
----------------
We shouldn't need to switch the R2/R3 variables over since CHECK-DAG takes care of ordering differences.


http://reviews.llvm.org/D21138