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

[Duncan Sands]

Hi Shuxin,

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

in my opinion the factorization code shouldn't crash, no matter what order you
visit instructions.  This can only be achieved by fixing 1), so I did that.  It
may be that the order you chose in your patch makes the correctness issue
impossible to hit, but that's not the point: fixing 1) makes it possible to
explore different ways of visiting instructions without having to worry about
reassociate blowing up underneath you: it makes reassociate more robust and more
easily maintainable.  Thus fixing 1) was a good thing to do in my opinion, so I
did it.

You seem upset that I ignored your patch and fixed the crash myself.  I'm sorry
you feel that way.  In my opinion your patch took the wrong approach to fixing
the crash.  That doesn't mean I think your patch takes the wrong approach to
improving how reassociate optimizes.  I consider that to be a different issue,
and I plan to examine it later.  I can certainly believe that visiting
instructions in a different order makes things better: the current order
doesn't make much sense, its main advantage is that it's simple.

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

These are not relevant to fixing the crash.

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

Woah, calm down!  I guess this proves I could have (should have) communicated
better.  Sorry about that.  I hope this email explains my actual viewpoint more
clearly.

Ciao, Duncan.

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