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

[Evan Cheng]

On Nov 5, 2012, at 10:47 PM, Shuxin Yang <shuxin.llvm at gmail.com> wrote:

> This patch is to fix rdar://12571717, it is about assertion in Reassociate pass.
> 
>  Long story short, the assertion is trigged when the pass try to transform expression
>     ... + 2 * n * 3 + 2 * m + ...
>  into:
>     ... + 2 * (n*3 + m).
> 
>  In the process of the transformation, a helper routine folds the constant 2*3 into 6,
> confusing optimizer which is trying the to eliminate the common factor 2, and cannot
> find 2 any more.
> 
>  Weaken the assertion doesn't help, as the transformation is half done.
> There are two ways to fix the probelm:
>  1) Ask the helper routine don't do better job, just do what it is expected.
>  2) Walk the expression worklist in a order such that sub-expr is visited prior to
>     the supper-expr. This way, 2*3 is already folded before the super-expr is visited.
> 
> 1) entails lots of change, 2) seems to be easier. It is just opposite:-(
> 
> I go for 2) not only because it seemed to be easier in the first place,
> I think it is the right order to go through expressions being reassociated.
> 
> The testing case (reduced by bugpoint) for this probelm is not inclued in the patch, as it
> is too big.
> 
> Thanks
> Shuxin
> 
> ================================================================================
> 
> Worklist
> ========
> 
>   The worklist WAS using SetVector. It was a container both for dead instructions and
> the expressions that could be optimized via reassociation. Exprs, no matter dead code
> or the those to be reassociated, are visited in FILO order.
> 
>   I rewrite the worklist. The new worklist has following traits:
>    o. it is basically std::deque
>    o. has "set" semantic, akin to the SetVector. Unlike the setVector, the
>       set is implemented using std::map instead of SmallVector for efficiency
>       purpose (In the case I'm working on, the worklist contains about 1.5k
>       instrucitons).
>    o. all dead instructions are eliminated in one loop: optimize iterates the
>       worklist in *backward* order, identifying the dead code, and
>       removing them "in-place". The "in-place" removal is done by invlaiding
>       the element instead of physically removing the space from the worklist.
>       This way, it obviates the need of memory move, and avoid invalidating
>       iterator.
>    o. the rest exprs are visited in forward order (i.e. FIFO)
> 
>   This part is implemented using STL style. It is so odd to see "PushBack" when
> we are getting used to "push_back":-)

Two quick questions:
1. Is it generic enough to expose the class as a LLVM ADT?
2. Does this patch have noticeable impact on compile time?

Thanks,

Evan

> 
> Other changes:
> ===============
>  The new worklist exposes some defects.
> 
>  1. test/Transforms/Reassociate/multistep.ll
>     Without my change a*a*b+a*a*c was turned into a*(a*(b+c)); with my
>     change the result is (a*a)*(b+c). I don't know for sure if the reassociator
>     intentionally choose tall-and-skinny expr tree in order to reduce register pressure.
> 
>  2. test/Transforms/Reassociate/mul_neg.ll (new)
> 
>     The reassociator was not able to perform following simplifiction:
>      (-x) * c => x * -c ,
>       -x * -y => -x * -y
>     if the negations are used in multiple placess.
> 
>    The optimization now is implemented by Reassociate::removeNegFromMulOps().
>    Without this opt, we will see three regressions.
> 
> <diff.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits