<div dir="ltr">I fixed my keyboard and sprung for some English, here's an updated patch. :-)</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jun 5, 2013 at 11:29 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">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;  // {1,8,16}=>{0,1,2}<br>
+  assert(Bitness < 3);<br>
+  bool hasV6Ops = Subtarget->hasV6Ops();<br>
+  bool isSingleInstr = 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 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>
<span class="HOEnZb"><font color="#888888"><br>
-eric<br>
</font></span><div class="HOEnZb"><div class="h5"><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 to<br>
> i8/i16/i32, with and without ARMv6, both in Thumb and ARM mode. This should<br>
> fix the bug as well as make FastISel faster because it bails to 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 for<br>
> 8-bit zext instead of UXTB. This simplifies code since it is supported on<br>
> ARMv4t and later, and at least on A15 both should perform exactly the 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 16178<br>
> repro.<br>
><br>
> fast-isel-ext.ll tests all sext/zext combinations that ARM FastISel 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 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>
</div></div></blockquote></div><br></div>