[PATCH] ARM FastISel integer sext/zext improvements

[Eric Christopher]

This will work, some comments on the patch itself:

+  using namespace ARM;

We don't really do this anywhere else...

+  static const uint8_t isSingleInstrTbl[3][2][2][2] = {
+  static const TargetRegisterClass *RCTbl[2][2] = {
+  static const struct {

Large explanatory block comments before each of these tables please.

+  unsigned SrcBits = SrcVT.getSizeInBits();
+  unsigned DestBits = DestVT.getSizeInBits();
+  assert(SrcBits < DestBits);
+  assert(DestBits == 32 || DestBits == 16 || DestBits == 8);
+  assert(SrcBits == 16 || SrcBits == 8 || SrcBits == 1);
+  unsigned Bitness = countTrailingZeros(SrcBits) >> 1;  // {1,8,16}=>{0,1,2}
+  assert(Bitness < 3);
+  bool hasV6Ops = Subtarget->hasV6Ops();
+  bool isSingleInstr = isSingleInstrTbl[Bitness][isThumb2][hasV6Ops][isZExt];
+  const TargetRegisterClass *RC = RCTbl[isThumb2][isSingleInstr];
+  unsigned Opc = OpcTbl[isSingleInstr][isThumb2][Bitness].Opc[isZExt];
+  assert(KILL != Opc && "Invalid table entry");
+  unsigned Imm = OpcTbl[isSingleInstr][isThumb2][Bitness].Imm[isZExt];
+  unsigned hasS = OpcTbl[isSingleInstr][isThumb2][Bitness].hasS[isZExt];
+  bool setsCPSR = &tGPRRegClass == RC;  // Always on 16-bit Thumb instructions.
+  unsigned LSLOpc = isThumb2 ? tLSLri : LSLi;
+  unsigned ResultReg;

Is your return key broken? ;)

Anyhow, some spacing here would be nice along with with a comment on
each of the asserts (or even text in the assert that says what it's
checking for ala the other asserts in llvm).

+  for (unsigned Instr = 0; Instr != (isSingleInstr ? 1 : 2); ++Instr) {

Brief comment here as well.

Thanks!

-eric

On Tue, Jun 4, 2013 at 6:06 PM, JF Bastien <jfb at google.com> wrote:
> My recent ARM FastISel patch exposed this bug:
>   http://llvm.org/bugs/show_bug.cgi?id=16178
> The root cause is that it can't select integer sext/zext pre-ARMv6 and
> asserts out.
>
> The current integer sext/zext code doesn't handle other cases gracefully
> either, so this patch makes it handle all sext and zext from i1/i8/i16 to
> i8/i16/i32, with and without ARMv6, both in Thumb and ARM mode. This should
> fix the bug as well as make FastISel faster because it bails to SelectionDAG
> less often. See fastisel-ext.patch for this.
>
> fastisel-ext-tests.patch changes current tests to always use reg-imm AND for
> 8-bit zext instead of UXTB. This simplifies code since it is supported on
> ARMv4t and later, and at least on A15 both should perform exactly the same
> (both have exec 1 uop 1, type I).
>
> 2013-05-31-char-shift-crash.ll is a bitcode version of the above bug 16178
> repro.
>
> fast-isel-ext.ll tests all sext/zext combinations that ARM FastISel should
> now handle.
>
> Note that my ARM FastISel enabling patch was reverted due to a separate
> failure when dealing with MCJIT, I'll fix this second failure and then turn
> FastISel on again for non-iOS ARM targets.
>
> I've tested "make check-all" on my x86 box, and "lnt test-suite" on A15
> hardware.