<div dir="ltr">Committed as r183551.</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jun 7, 2013 at 11:17 AM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Sure. Have at.<br>
<span class="HOEnZb"><font color="#888888"><br>
-eric<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Wed, Jun 5, 2013 at 6:04 PM, JF Bastien <<a href="mailto:jfb@google.com">jfb@google.com</a>> wrote:<br>
> I fixed my keyboard and sprung for some English, here's an updated patch.<br>
> :-)<br>
><br>
><br>
> On Wed, Jun 5, 2013 at 11:29 AM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>><br>
> wrote:<br>
>><br>
>> This will work, some comments on the patch itself:<br>
>><br>
>> +  using namespace ARM;<br>
>><br>
>> We don't really do this anywhere else...<br>
>><br>
>> +  static const uint8_t isSingleInstrTbl[3][2][2][2] = {<br>
>> +  static const TargetRegisterClass *RCTbl[2][2] = {<br>
>> +  static const struct {<br>
>><br>
>> Large explanatory block comments before each of these tables please.<br>
>><br>
>> +  unsigned SrcBits = SrcVT.getSizeInBits();<br>
>> +  unsigned DestBits = DestVT.getSizeInBits();<br>
>> +  assert(SrcBits < DestBits);<br>
>> +  assert(DestBits == 32 || DestBits == 16 || DestBits == 8);<br>
>> +  assert(SrcBits == 16 || SrcBits == 8 || SrcBits == 1);<br>
>> +  unsigned Bitness = countTrailingZeros(SrcBits) >> 1;  //<br>
>> {1,8,16}=>{0,1,2}<br>
>> +  assert(Bitness < 3);<br>
>> +  bool hasV6Ops = Subtarget->hasV6Ops();<br>
>> +  bool isSingleInstr =<br>
>> isSingleInstrTbl[Bitness][isThumb2][hasV6Ops][isZExt];<br>
>> +  const TargetRegisterClass *RC = RCTbl[isThumb2][isSingleInstr];<br>
>> +  unsigned Opc = OpcTbl[isSingleInstr][isThumb2][Bitness].Opc[isZExt];<br>
>> +  assert(KILL != Opc && "Invalid table entry");<br>
>> +  unsigned Imm = OpcTbl[isSingleInstr][isThumb2][Bitness].Imm[isZExt];<br>
>> +  unsigned hasS = OpcTbl[isSingleInstr][isThumb2][Bitness].hasS[isZExt];<br>
>> +  bool setsCPSR = &tGPRRegClass == RC;  // Always on 16-bit Thumb<br>
>> instructions.<br>
>> +  unsigned LSLOpc = isThumb2 ? tLSLri : LSLi;<br>
>> +  unsigned ResultReg;<br>
>><br>
>> Is your return key broken? ;)<br>
>><br>
>> Anyhow, some spacing here would be nice along with with a comment on<br>
>> each of the asserts (or even text in the assert that says what it's<br>
>> checking for ala the other asserts in llvm).<br>
>><br>
>> +  for (unsigned Instr = 0; Instr != (isSingleInstr ? 1 : 2); ++Instr) {<br>
>><br>
>> Brief comment here as well.<br>
>><br>
>> Thanks!<br>
>><br>
>> -eric<br>
>><br>
>> On Tue, Jun 4, 2013 at 6:06 PM, JF Bastien <<a href="mailto:jfb@google.com">jfb@google.com</a>> wrote:<br>
>> > My recent ARM FastISel patch exposed this bug:<br>
>> >   <a href="http://llvm.org/bugs/show_bug.cgi?id=16178" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=16178</a><br>
>> > The root cause is that it can't select integer sext/zext pre-ARMv6 and<br>
>> > asserts out.<br>
>> ><br>
>> > The current integer sext/zext code doesn't handle other cases gracefully<br>
>> > either, so this patch makes it handle all sext and zext from i1/i8/i16<br>
>> > to<br>
>> > i8/i16/i32, with and without ARMv6, both in Thumb and ARM mode. This<br>
>> > should<br>
>> > fix the bug as well as make FastISel faster because it bails to<br>
>> > SelectionDAG<br>
>> > less often. See fastisel-ext.patch for this.<br>
>> ><br>
>> > fastisel-ext-tests.patch changes current tests to always use reg-imm AND<br>
>> > for<br>
>> > 8-bit zext instead of UXTB. This simplifies code since it is supported<br>
>> > on<br>
>> > ARMv4t and later, and at least on A15 both should perform exactly the<br>
>> > same<br>
>> > (both have exec 1 uop 1, type I).<br>
>> ><br>
>> > 2013-05-31-char-shift-crash.ll is a bitcode version of the above bug<br>
>> > 16178<br>
>> > repro.<br>
>> ><br>
>> > fast-isel-ext.ll tests all sext/zext combinations that ARM FastISel<br>
>> > should<br>
>> > now handle.<br>
>> ><br>
>> > Note that my ARM FastISel enabling patch was reverted due to a separate<br>
>> > failure when dealing with MCJIT, I'll fix this second failure and then<br>
>> > turn<br>
>> > FastISel on again for non-iOS ARM targets.<br>
>> ><br>
>> > I've tested "make check-all" on my x86 box, and "lnt test-suite" on A15<br>
>> > hardware.<br>
><br>
><br>
</div></div></blockquote></div><br></div>