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

[Shuxin Yang]

  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":-)

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: diff.patch
Type: text/x-patch
Size: 13987 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121105/df847165/attachment.bin>