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

Shuxin Yang shuxin.llvm at gmail.com
Thu Nov 15 11:17:12 PST 2012


With latest revision, the performance defect exposed by this snippet 
remains:

; RUN: opt -S -reassociate < %s | FileCheck %s

; t=-a; retval = t*7|t => t-a; retval => a*-7|t
define i32 @mulneg(i32 %a) nounwind uwtable ssp {
entry:
   %sub = sub nsw i32 0, %a
   %tmp1 = mul i32 %sub, 7
   %tmp2 = xor i32 %sub, %tmp1
   ret i32 %tmp2
; CHECK: entry
; CHECK: %tmp1 = mul i32 %a, -7
; CHECK: ret
}


On 11/15/12 10:51 AM, Evan Cheng wrote:
> 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




More information about the llvm-commits mailing list