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

[Duncan Sands]

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
>