[PATCH] Implement shift ops for Mips fast-isel

[Daniel Sanders]

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1297
@@ +1296,3 @@
+  switch (DVT.SimpleTy) {
+  default:
+    // Trunc i64 to i32 is handled by the target-independent fast-isel.
----------------
echristo wrote:
> rkotler wrote:
> > echristo wrote:
> > > I'd add this assert:
> > > 
> > > assert((DVT.SimpleTy != MVT::i32 && DVT.SimpleTy != MVT::i64) && "i64 and i32 should have been handled by target-independent fast-isel!");
> > If we want to add that then why not just a simple LLVM_Unreachable ??
> Because SimpleTy are more than the two you have there and you'd like to make sure that all of the possible types that come in are handled rather than say that you've handled all of them?
I assume Reed meant something like:
  default:
    return false;
  case MVT::i32:
  case MVT::i64:
    llvm_unreachable("Text");
    return false;

I was recently informed that llvm_unreachable() is pretty much the same thing as assert(0 && "Text") and that they are removed in release builds (and even informs the optimizer that the path is unreachable). On that basis, an assert() is slightly preferable to a llvm_unreachable().

================
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);
----------------
rkotler wrote:
> rkotler wrote:
> > dsanders wrote:
> > > 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.
> > This is definitely there to fix some failure in test-suite that will show up without this. It's clearly not incorrect to add this code. The whole way that fast-isel skirts around not having a legalizer is dubious at times. I don't know how easy it will be to create a separate test case for this. So it's a problem with an earlier patch but only shows up if you add this patch to it.
> > 
> > So we clearly cannot check in the rest of the patch without this because test-suite will have failures. So I consider it to be part of implementing this patch.
> My recollection is that other fast-isel ports do not implement the shift ops for non legalized types which is why they are not seeing this issue.
A separate test case is ideal, but pointing me at the test-suite failure and the bad assembly within it will suffice.

I'm not saying that I think the zero extension is unnecessary, I just suspect that the wrong LLVM-IR expansion is doing it.
My suspicion is that the consumer of the value is reading the whole register instead of either forcing a zero extension itself or reading just the bits it expects to operate on. It could well be selectShift() since it currently doesn't zero-extend the operand for a LShr.

For example:
  %0 = trunc i32 %a to i16
  %1 = trunc i32 %b to i16
  %2 = add i16 %0, %1
clearly doesn't need either of the zero extensions this code will currently create but:
  %0 = trunc i32 %a to i16
  %1 = trunc i32 %b to i16
  %2 = lshr i16 %0, i32 3
needs lshr to zero extend the input register before use.


================
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))) {
----------------
rkotler wrote:
> dsanders wrote:
> > 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.
> Yes, the comment needs to be moved above.
> 
> I don't think we need sign extension unless we have AShr. What is in the register is already an 8, 16 bit value. It's as if there was an 8, 16 bit register. These are treated as if they are legal types but they are not really legal. We want to use a 32 bit form of shift which will have the right semantics unless it's an AShr.
> 
> In the case of an Ashr, it will not behave properly unless we have sign extended the value into a 32 bit register.
> 
> This  patch BTW passes all of test suite and there are tests where getting this semantics right counts.
>> 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.
> I don't think we need sign extension unless we have AShr. What is in the register is already an 8, 16 bit value. It's as if there was an 8, 16 bit
> register. These are treated as if they are legal types but they are not really legal. We want to use a 32 bit form of shift which will have the right
> semantics unless it's an AShr.
>
> In the case of an Ashr, it will not behave properly unless we have sign extended the value into a 32 bit register.

I agree with the arguments and explanation here but I think you may have missed my key question. The question I'm trying to ask is: Why doesn't LShr zero extend the input register just like AShr sign extends the input register? Similarly to AShr it will be implementing this with a 32-bit shift right and surely therefore requires zero extension to i32 to happen first. I suspect the reason this currently passes the test suite is because the code in selectTrunc() that I've raised comments about is concealing the bug in selectShift() for LShr.

The secondary part of the comment is that we also don't need to sign-extend if the operand in question is already a suitable sign-extend operation.

http://reviews.llvm.org/D6726

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