[PATCH] Implement mips fast-isel select statement

Daniel Sanders daniel.sanders at imgtec.com
Tue May 12 04:06:38 PDT 2015


================
Comment at: lib/Target/Mips/MipsFastISel.cpp:928-934
@@ +927,9 @@
+    return false;
+  case MVT::i1:
+  case MVT::i8:
+  case MVT::i16:
+  case MVT::i32:
+    CondMovOpc = Mips::MOVN_I_I;
+    RC = &Mips::GPR32RegClass;
+    break;
+  case MVT::f32:
----------------
We don't need to check each integer type explictly. The generated code is correct for all integers of GPR size or smaller.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:947
@@ +946,3 @@
+  const Value *Cond = SI->getCondition();
+  (void)Cond;
+  unsigned Src1Reg = getRegForValue(SI->getTrueValue());
----------------
Nit: Useless statement

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:960
@@ +959,3 @@
+    return false;
+  emitInst(TargetOpcode::COPY, TempReg).addReg(Src2Reg);
+  MachineInstrBuilder MI = emitInst(CondMovOpc, ResultReg)
----------------
I think this is only needed if Src2Reg != DestReg. Is it possible for this code to emit something like:
  move $2, $2
  movn $2, $3, $4

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:964
@@ +963,3 @@
+                               .addReg(CondReg)
+                               .addReg(TempReg, RegState::Implicit);
+  MI->tieOperands(0, 3);
----------------
TempReg isn't implicitly read. It's explicit in our instruction definition:
  (outs DRC:$rd), (ins DRC:$rs, CRC:$rt, DRC:$F)

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1660-1679
@@ -1600,2 +1659,22 @@
 
+unsigned MipsFastISel::fastEmitInst_rr(unsigned MachineInstOpcode,
+                                       const TargetRegisterClass *RC,
+                                       unsigned Op0, bool Op0IsKill,
+                                       unsigned Op1, bool Op1IsKill) {
+  if (MachineInstOpcode == Mips::MUL) {
+    unsigned ResultReg = createResultReg(RC);
+    const MCInstrDesc &II = TII.get(MachineInstOpcode);
+    Op0 = constrainOperandRegClass(II, Op0, II.getNumDefs());
+    Op1 = constrainOperandRegClass(II, Op1, II.getNumDefs() + 1);
+    BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, II, ResultReg)
+        .addReg(Op0, getKillRegState(Op0IsKill))
+        .addReg(Op1, getKillRegState(Op1IsKill))
+        .addReg(Mips::HI0, RegState::ImplicitDefine | RegState::Dead)
+        .addReg(Mips::LO0, RegState::ImplicitDefine | RegState::Dead);
+    return ResultReg;
+  }
+  return FastISel::fastEmitInst_rr(MachineInstOpcode, RC, Op0, Op0IsKill, Op1,
+                                   Op1IsKill);
+}
+
 namespace llvm {
----------------
Why is this chunk in this patch? It looks we have two patches in one review.

================
Comment at: test/CodeGen/Mips/Fast-ISel/sel1.ll:22
@@ +21,3 @@
+; Function Attrs: noinline nounwind
+define void @foo() #0 {
+entry:
----------------
At this point we should have support for arguments and returns. Could you simplify the test using those?

I don't mind if we clean this up later but I'd like to have as little noise as possible in the tests.

http://reviews.llvm.org/D6774

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list