<div dir="ltr">Hi Daniel, <div><br>I presume you mean, instead of assigning function arguments distinct ranks (<a href="http://llvm.org/docs/doxygen/html/Reassociate_8cpp_source.html#l00282">http://llvm.org/docs/doxygen/html/Reassociate_8cpp_source.html#l00282</a>), we should group function arguments in favor of existing pairings. You are not suggesting discarding the entire ranking system, right? </div><div><br></div><div>I'll look into how that works on my benchmarks. AFAIK, we encountered some cases that seem beyond the fix you suggested. These cases involve constants, and I attached one reduced example in <a href="https://llvm.org/bugs/show_bug.cgi?id=22357">https://llvm.org/bugs/show_bug.cgi?id=22357</a>. </div><div><br>void foo(int a, int b, int c, int *x, int *y) {<br>  *x = (a + b);</div><div>  *y = (a + 2) + b;</div><div>}
<br><br>Reassociate assigns constants a lower rank than variables, which prevents Reassociate from transforming the above example to</div><div><br></div><div>*x = a + b;</div><div>*y = (a + b) + 2;<br><br>Jingyue</div><div><br><div class="gmail_quote">On Mon, May 4, 2015 at 4:45 PM Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Whoops, forgot llvmdev<br>
<br>
<br>
On Mon, May 4, 2015 at 4:12 PM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>> wrote:<br>
> So i started by looking at naryreassociate, whose pass<br>
> description/reason listed for doing it is actually describes bug in<br>
> reassociate, and discovered that, in fact, reassociate seems broken,<br>
> and should be doing the right thing on most of your testcases.<br>
><br>
> Let's take nary-add.ll, left_reassociate<br>
><br>
> ; RUN: opt < %s -nary-reassociate -S | FileCheck %s<br>
><br>
> target datalayout = "e-i64:64-v16:16-v32:32-n16:32:64"<br>
><br>
> declare void @foo(i32)<br>
><br>
> ; foo(a + c);<br>
> ; foo((a + (b + c));<br>
> ;   =><br>
> ; t = a + c;<br>
> ; foo(t);<br>
> ; foo(t + b);<br>
> define void @left_reassociate(i32 %a, i32 %b, i32 %c) {<br>
>   %1 = add i32 %a, %c<br>
>   call void @foo(i32 %1)<br>
>   %2 = add i32 %b, %c<br>
>   %3 = add i32 %a, %2<br>
>   call void @foo(i32 %3)<br>
>   ret void<br>
> }<br>
><br>
><br>
> normal reassociate transforms this, IMHO, wrongly, into:<br>
><br>
> define void @left_reassociate(i32 %a, i32 %b, i32 %c) {<br>
>   %1 = add i32 %c, %a<br>
>   call void @foo(i32 %1)<br>
>   %2 = add i32 %b, %a<br>
>   %3 = add i32 %2, %c<br>
>   call void @foo(i32 %3)<br>
>   ret void<br>
> }<br>
><br>
><br>
> This is because for the first expression:<br>
><br>
> RAIn: add i32 [ %a, #3] [ %c, #5]<br>
> RAOut: add i32 [ %c, #5] [ %a, #3]<br>
><br>
><br>
> and<br>
> for the second:<br>
> RAIn: add i32 [ %a, #3] [ %b, #4] [ %c, #5]<br>
> RAOut: add i32 [ %c, #5] [ %b, #4] [ %a, #3]<br>
><br>
><br>
> This makes it transform the first into add c, a<br>
> and the second into<br>
> %1 = add b, a<br>
> add c, %1<br>
><br>
><br>
> This is caused by these arguments having different ranks (because they<br>
> are function arguments), and it not respecting the same order it has<br>
> already chosen once.<br>
><br>
><br>
> This is, IMHO, pretty clearly a bug.<br>
><br>
> It screws up right_reassociate, no_reassociate, and conditional tests<br>
> for the same reason.<br>
><br>
> If you fix this, i expect you will find a lot less use cases for<br>
> nary-reassociate.<br>
><br>
><br>
> The simple way to fix this is to mark which operands it's paired<br>
> together in the past, and always try to pair the same ones together if<br>
> it can, regardless of rank.<br>
<br>
Or simply adjust the ranks of paired operands to be the same and<br>
different from all other ranks<br>
<br>
(BBrank is << 16, so you should have room do to this)<br>
</blockquote></div></div></div>