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

[Evan Cheng]

On Nov 15, 2012, at 10:41 AM, Shuxin Yang <shuxin.llvm at gmail.com> wrote:

> 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.

There is no such thing as "exclusive" write permission in the LLVM community. I don't think this is ever suggested during the discussion.

Evan

> 
> 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
>>> 
>> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits