[PATCH] Implement shift ops for Mips fast-isel

reed kotler rkotler at mips.com
Thu Feb 26 18:23:47 PST 2015


I am working on the make check test. Attached is the C code that was the original test case.F407541: shftop.c <http://reviews.llvm.org/F407541>


================
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);
----------------
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.

================
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:
> 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.

================
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))) {
----------------
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.

================
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;
----------------
dsanders wrote:
> Is this comment still true? You already have sign extension code for operand 0 above.
Correct. The comment does not belong anymore.

http://reviews.llvm.org/D6726

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






More information about the llvm-commits mailing list