<div dir="ltr">Hi Meador,<div><br></div><div>Thanks for having a look. Ouch, I'll fix that crash pronto, thanks!</div><div><br></div><div>> <span style="line-height:1.5">I haven't worked through all the logic yet, but this seems like a very specific optimization.  How often have you found that it fires in practice?  Any benchmark results?</span></div><div><span style="line-height:1.5"><br></span></div><div><span style="line-height:1.5">I'd counter that it is the obvious optimization to select that instruction. At the moment there's no way to get RBIT selected apart from by using an intrinsic. This at least makes the obvious bit-reversal code in C do the right thing. I've found it firing at least a couple of times in the test-suite, and very much so in a third-party benchmark suite that I can't share. In that suite I get about a 200% speedup in one test with this optimization.</span></div><div><span style="line-height:1.5"><br></span></div><div><span style="line-height:1.5">I'm happy to change the capitalization - it was mainly force of habit.</span></div><div><span style="line-height:1.5"><br></span></div><div><span style="line-height:1.5">Cheers,</span></div><div><span style="line-height:1.5"><br></span></div><div><span style="line-height:1.5">James</span></div></div><br><div class="gmail_quote"><div dir="ltr">On Mon, 2 Nov 2015 at 16:27 Meador Inge via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">meadori added a subscriber: meadori.<br>
meadori added a comment.<br>
<br>
I haven't worked through all the logic yet, but this seems like a very specific optimization.  How often have you found that it fires in practice?  Any benchmark results?<br>
<br>
Also, changing the test case to:<br>
<br>
  --- test.ll   2015-11-02 10:21:04.000000000 -0600<br>
  +++ crash.ll  2015-11-02 10:20:51.000000000 -0600<br>
  @@ -11,7 +11,7 @@<br>
   entry:<br>
     %a.lobit = lshr i32 %a, 31<br>
     %and1 = lshr i32 %a, 29<br>
  -  %0 = and i32 %and1, 2<br>
  +  %0 = and i32 %and1, 1<br>
     %and6 = lshr i32 %a, 27<br>
     %1 = and i32 %and6, 4<br>
     %and11 = lshr i32 %a, 25<br>
<br>
causes the following assert to fire:<br>
<br>
  Assertion failed: (bitPosition < getBitWidth() && "Bit position out of bounds!")<br>
<br>
<br>
================<br>
Comment at: lib/Target/ARM/ARMISelLowering.cpp:8896<br>
@@ +8895,3 @@<br>
+// Try and detect an RBIT - bit reversal.<br>
+static SDValue performRBITCombine(SDNode *N,<br>
+                                  TargetLowering::DAGCombinerInfo &DCI,<br>
----------------<br>
Any particular reason this is `performRBITCombine` instead of `PerformRBITCombine`?  The coding standard calls for functions to start with lowercase letters, but it looks like most of the functions in this file are already start with uppercase letters.<br>
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="http://reviews.llvm.org/D14234" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14234</a><br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>