[llvm-commits] [PATCH][SOPT] Fix assertion in Reassociate

[Shuxin Yang]

Hi, Dancan:

     As I state in my mail sent to this list for review. I already 
stated there are two ways to move on:
     1) to fix the "problem" of LinearizeExprTree -- the problem is that 
it does better job than it is expected.
     2) choose right order of optimization.

    I don't like to touch 1) because it seems to be too complicate then 
the concept involved. As far as I can understand
it just to decompose a big expression tree into N-ary form, like E1 + E2 
... + E2. This function can reused for other reassociates
for different purpose: like reassociate for tree-height-reduction, 
reassociate to expose redundancy ( a + b + c => (a + c) + b,
it there is a+c in the nearby code).

    I went for 2) because iterating the instructions in right order is 
important to the reassociate performance.

    1) and 2) are *NOT* exclusive.

    2) not only fix (you may think it is merely sidestep) the problem.
       It include the enhancement to the worklist, making it possible to 
go through the worklist in forward order.
       It also remove the cost the erasing element in the middle of the 
container.
       It also include the optimization for "t = -a; t2 = t * 7 (where t 
has multiple uses)" => t2 = a * -7"

     How about all this changes?

    Down the load, please let us know if you want to own the 
Reassociate.cpp and have  *EXCLUSIVELY" write-permission or not.

Thanks
Shuxin


On 11/15/12 2:04 AM, Duncan Sands wrote:
> Hi Shuxin,
>
> On 06/11/12 20:43, Shuxin Yang wrote:
>> Hi, Duncan:
>>
>>      Thank you for looking into this problem.
>>
>>      I don't know for sure if the testing case I'm working on is 
>> proprietary or
>> not.
>> I try to come up a contrived example, see bellow.  This contrived 
>> example triggers
>> exactly the same assertion, but the assertion is triggered at 
>> different stage.
>> (See following
>> code for the concept of stage).
>
> thanks for the testcase.  I've fixed the issue in commit 168035. This 
> is a
> mismatch between what RemoveFactorFromExpression is expecting 
> LinearizeExprTree
> to do, and what LinearizeExprTree actually does (it does too much).  
> As far as
> I can see this is an accident waiting to happen, so better to make 
> things more
> robust rather than trying to dance carefully around it by changing the 
> order in
> which instructions are visited.  I hope this fixes the real testcase too.
>
> Ciao, Duncan.
>
>>
>> contrived example
>> =============
>> cat a.ll
>> define i32 @foo(i32 %arg, i32 %arg1, i32 %arg2) nounwind {
>> mylab:
>>    %tmp1 = mul i32 %arg1, 2
>>    %tmp2 = mul i32 %tmp1, 3
>>    %tmp3 = mul i32 %arg2, 2
>>   ; dead code
>>    %tmp4 = add i32 %tmp1, 1
>>    %ret = add i32 %tmp2, %tmp3
>>    ret i32 %ret
>> }
>>
>> reproduced by "opt -S -reassociate a.ll
>>
>>    Unfortunately, my patch does not catch this contrived example 
>> either, because
>> it only tries to catch this situation
>> in stage 2. see bellow:
>>
>>   The "stages"
>>   =======================
>>   bool Reassociate::runOnFunction() {
>>     for each basic block bb {
>>         // stage 1: go through all instruction in given bb in forward 
>> order
>>          for each instruction i
>>              if i is dead
>>                  erase i
>>              else
>>                  reassociate i
>>
>>           // stage 2:
>>           while (worklist != empty) {
>>                i = worklist.pop_back_val;
>>                opt or delete i;
>>           }
>>    }
>>
>>   I guess we should not delete Instruction during the process of 
>> reassociation.
>>
>> Thanks
>> Shuxin
>>
>>
>> On 11/6/12 1:20 AM, Duncan Sands wrote:
>>> Hi Shuxin,
>>>
>>>>   The testing case (reduced by bugpoint) for this probelm is not 
>>>> inclued in the
>>>> patch, as it
>>>> is too big.
>>>
>>> can you please send it to the list so that people can analyse it, 
>>> even if it
>>> won't be part of the final patch.
>>>
>>> Thanks, Duncan.
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>