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

[Shuxin Yang]

Hi, Duncan:

     As far as I can understand of this problem and your email, we kind 
of agree on the observation
that the root cause is the miscommunication between two functions. For 
convenience, let us say function A and B.
where A is a utility routine, it tries to decompose a big expression 
tree (DAG actually) into a
N-ary form, in the meantime, it tires to do perform some optimization 
whenever possible.

    If miscommunication take place, and each side in correct on its own, 
who is culprit? I think both or neither.

   The miscommunication arise when A() tries to perform optimization, 
which beyond the caller's expectation.
However,A() is called in many places, as far as I can understand, in 
some places, it is desirable to perform
as good job as possible.

   I personally don't like that a utility routine has any decision 
power, it would make life easier to shift
decision power to the driver/optimizer side. Of course, this is just a 
personal taste. In this case,  shifting
decision power from utility routine A() to the optimizer pretty much 
requires rewrite the whole Reassociate
from ground up.

   As far as I can tell from your change, it is just disable constant 
folding in A(). While A() is less smarter
than it was, it still is smart. I'm wondering if your change fixes the 
root cause of a set of problems exposed
by this specific problem, or just fix this specific problem.

   I agree, my fixes dose not "fix" the root cause , but iterating 
instructions in forward order making it
possible to optimize in the order of sub-expression to supper 
expression. This way,  although A() is smart,
it has no chance to showcase its capability and hence eschew the problem.

Shuxin

On 11/15/12 12:39 PM, Duncan Sands wrote:
> 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
>>>>
>>>
>>
>