[llvm-commits] [PATCH][SOPT] Fix assertion in Reassociate
Evan Cheng
evan.cheng at apple.com
Tue Nov 6 16:30:08 PST 2012
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
More information about the llvm-commits
mailing list