[PATCH] ARM FastISel integer sext/zext improvements

[JF Bastien]

Committed as r183551.


On Fri, Jun 7, 2013 at 11:17 AM, Eric Christopher <echristo at gmail.com>wrote:

> Sure. Have at.
>
> -eric
>
> On Wed, Jun 5, 2013 at 6:04 PM, JF Bastien <jfb at google.com> wrote:
> > I fixed my keyboard and sprung for some English, here's an updated patch.
> > :-)
> >
> >
> > On Wed, Jun 5, 2013 at 11:29 AM, Eric Christopher <echristo at gmail.com>
> > wrote:
> >>
> >> 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.
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130607/f9a1adfa/attachment.html>