[PATCH] Add sign/zero extend to mips fast-isel

[Daniel Sanders]

>>! In D4827#14, @mcrosier wrote:
>>>! In D4827#12, @rkotler wrote:
>> I don't think that any of the proposed changes to my patch are necessary and I don't really agree with them helping the code and to the contrary I think they just make it more complicated and relying on things that may not always be true even, in the future.
> 
> I very much agree with Reid.

I disagree but to a greater or lesser extent according to the change.  I'll reply to each one inline with the reason for the request but the overarching principle is that I'd prefer to not to leave gaps in the implementation that need rediscovering at a later point since this has been a major source of pain in both the integrated assembler and code generator. The use of SEB/SEH (which requires MIPS32r2/MIPS64r2) without handling MIPS32r1/MIPS64r1 and earlier is a good example of this.

>> Regarding not using the output of clang; this is something i can look into but i'm more comfortable at this time doing it the way I'm doing it and would have to get better at writing LLVM IR by hand.
> 
> I don't think this is a serious blocker.

Indeed, it's not a serious blocker for an individual patch. I brought it up because I'd prefer that our test cases omit the unnecessary artifacts that clang introduces for its own convenience.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:252-264
@@ +251,15 @@
+                               unsigned DestReg) {
+  if ((DestVT != MVT::i32) && (DestVT != MVT::i16))
+    return false;
+  switch (SrcVT.SimpleTy) {
+  default:
+    return false;
+  case MVT::i8:
+    EmitInst(Mips::SEB, DestReg).addReg(SrcReg);
+    break;
+  case MVT::i16:
+    EmitInst(Mips::SEH, DestReg).addReg(SrcReg);
+    break;
+  }
+  return true;
+}
----------------
dsanders wrote:
> I think we could better write this as something like:
>   unsigned SizeInBits = SrcVT.getSizeInBits();
>   if (SizeInBits > GPRWidthInBits)
>     return false;
>   if (SizeInBits == 8 && isMips32r2()) {
>     EmitInst(Mips::SEB, DestReg).addReg(SrcReg);
>     return true;
>   }
>   if (SizeInBits == 16 && isMips32r2()) {
>     EmitInst(Mips::SEH, DestReg).addReg(SrcReg);
>     return true;
>   }
>   EmitInst(Mips::SLL, DestReg).addReg(SrcReg).addImm(GPRWidth - SizeInBits); 
>   EmitInst(Mips::SRA, DestReg).addReg(DestReg).addImm(GPRWidth - SizeInBits);
>   return true;
> rather than listing each case. This looks a bit longer but it handles MIPS32r1/MIPS64r2 (and earlier), as well as unusual types such as i11 using the SLL/SRA. Meanwhile, it still provides the faster output when SEB/SEH are available.
> 
> (I realize I'm violating SSA form in the last three lines of the example but it's just for explanation and should be easy to fix)
The intention of this request is for correctness for multiple ISA's. The code in the original patch is using instructions that only exist on MIPS32r2/MIPS64r2 and above. The request is to support all known MIPS ISA's with a trivial change by falling back on an SHL/SRA sequence.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:269-281
@@ +268,15 @@
+                               unsigned DestReg) {
+  switch (SrcVT.SimpleTy) {
+  default:
+    return false;
+  case MVT::i1:
+    EmitInst(Mips::ANDi, DestReg).addReg(SrcReg).addImm(1);
+    break;
+  case MVT::i8:
+    EmitInst(Mips::ANDi, DestReg).addReg(SrcReg).addImm(0xff);
+    break;
+  case MVT::i16:
+    EmitInst(Mips::ANDi, DestReg).addReg(SrcReg).addImm(0xffff);
+    break;
+  }
+  return true;
----------------
dsanders wrote:
> I think we could simplify this to something like:
>   if (SrcVT.getSizeInBits() <= 16)
>     unsigned Mask = APInt::getMaxValue(SrcVT.getSizeInBits()).getZExtValue();
>     EmitInst(Mips::ANDi, DestReg).addReg(SrcReg).addImm(Mask);
>     return true;
>   }
>   return false;
> 
> rather than listing each case. It handles unusual types like i9 too.
This one is refactoring. A 16-bit immediate and instruction is available on all MIPS ISA's and it seemed strange to special case three particular sizes when a single calculation would do the same job.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:389-392
@@ +388,6 @@
+
+  if (SrcVT != MVT::i32 && SrcVT != MVT::i16 && SrcVT != MVT::i8)
+    return false;
+  if (DestVT != MVT::i16 && DestVT != MVT::i8 && DestVT != MVT::i1)
+    return false;
+
----------------
dsanders wrote:
> Given that trunc is only valid for integers and vectors of integers, do we need to test the type combinations?
> 
> I think this this will suffice:
>   if (SrcVT.isVector() || DestVT.isVector())
>     return false;
> possibly with a check that the size in bits is <= 32
This one is a query. It looked like the suggested code was equivalent and would remain so in the future. However, I couldn't be certain.

http://reviews.llvm.org/D4827