[PATCH] Implement shift ops for Mips fast-isel

[Daniel Sanders]

Testcase? Also, one common nit is that many of the comments lack capitalization.

>From the patch summary:

> Am reviewing this myself now but passes all of test-suite.


The patch summary is supposed to be the commit message you intend to use. This kind of thing ought to be a follow up comment.


================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1304-1324
@@ -1289,4 +1303,23 @@
 
-  // Because the high bits are undefined, a truncate doesn't generate
-  // any code.
+  uint64_t Mask = 0;
+  MVT DVT = DestVT.getSimpleVT();
+  switch (DVT.SimpleTy) {
+  default:
+    // Trunc i64 to i32 is handled by the target-independent fast-isel.
+    return false;
+  case MVT::i1:
+    Mask = 0x1;
+    break;
+  case MVT::i8:
+    Mask = 0xff;
+    break;
+  case MVT::i16:
+    Mask = 0xffff;
+    break;
+  }
+  if (Mask != 0) {
+    unsigned DestReg = createResultReg(&Mips::GPR32RegClass);
+    emitInst(Mips::ANDi, DestReg).addReg(SrcReg).addImm(Mask);
+    SrcReg = DestReg;
+  }
   updateValueMap(I, SrcReg);
----------------
Can you show me a test case that requires this? At the moment, the old code sounds correct to me and I think the consumers of this value should be sign/zero extending if they care about particular values of the undefined portion of the register.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1430
@@ -1396,1 +1429,3 @@
 
+bool MipsFastISel::selectShift(const Instruction *I) {
+  MVT RetVT;
----------------
The first section of code needs some vertical whitespace to break up the logical parts. It's a page of solid code at the moment.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1443-1449
@@ +1442,9 @@
+  if (Opcode == Instruction::AShr) {
+    unsigned TempReg = createResultReg(&Mips::GPR32RegClass);
+    if (!TempReg)
+      return false;
+    MVT Op0MVT = TLI.getValueType(Op0->getType(), true).getSimpleVT();
+    if (!emitIntSExt(Op0MVT, Op0Reg, MVT::i32, TempReg))
+      return false;
+    Op0Reg = TempReg;
+  }
----------------
Just a comment:
I expect a lot of clutter could be eliminated by moving the createResultReg() and associated code inside the emit*() functions and returning the register or 0 for failure.

Then this code here would be something like:
  MVT Op0MVT = ...;
  Op0Reg = emitIntSExt(Op0MVT, ...);
  if (!Op0Reg)
    return false;

It also allows the emit functions to handle nops by returning an operand register and constant 0 by returning $zero which will be useful in some cases.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1451-1452
@@ +1450,4 @@
+  }
+  // if AShr then we need to make sure the operand0 is sign extended
+  //
+  if (const auto *C = dyn_cast<ConstantInt>(I->getOperand(1))) {
----------------
Shouldn't this comment come just before the if-statement on line 1442?

Also, I don't think this is limited to AShr and we don't need to do it if the operand came from a SExtInst. I think LShr ought to be zero extending unless the operand comes from a ZExtInst.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1463-1465
@@ +1462,5 @@
+      Opcode = Mips::SRA;
+      // this requires some extra work and so far
+      // it is never called. we need to sign extend Op0 before
+      // doing the shift if it is i8 or i16.
+      break;
----------------
Is this comment still true? You already have sign extension code for operand 0 above.

http://reviews.llvm.org/D6726

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