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

[Shuxin Yang]

>Two quick questions:
>1. Is it generic enough to expose the class as a LLVM ADT?
Actually I used template to implement this worklist at beginning, hoping it will be promoted into ADT.
I were blocked at the in-place removal. The value_type is actually AssertingVH<Instruction> which is
kinda smartpointer of Instruction*.

In the code excerpted bellow, if I replace the "Instruction*" (@ line 378) with the value_type,
I will see assertion somewhere. The reason is that the destructor of "t" needs to be called
before cb(), which is impossible.


369 // inplace_rremove() is the same as inplace_remove() except that elements
  370 // are visited in backward order.
  371 template <typename pred, typename call_back_func>
  372 int RedoWorklist::inplace_rremove(pred p, call_back_func cb) {
  373   int cnt = 0;
  374   for (typename deque_type::reverse_iterator iter = deque_.rbegin(),
  375        iter_e = deque_.rend(); iter != iter_e; iter++) {
  376     value_type &element = *iter;
  377     if (p(element) && set_.erase(element)) {
  378       Instruction* t = element;
  379       element.~value_type();
  380       new (&element) value_type(NULL);
  381       cb(t);
  382       cnt ++;
  383     }
  384   }
  385   return cnt;
  386 }
  387
  
The original code does not have problem, see following code

     Worklist<<AssertingVH<Instruction> > Worklist;
     
     Instruction* I = Worklist.pop_front_val(); // the AssertingVH<Instruction>::~AssertingVH<Instruction> is called right after this stmt.
  



> 2. Does this patch have noticeable impact on compile time?

  It would be noticeable slower if I use std::list and std::map combination.
I don't know if my implementation is faster or slower than the original worklist (using SetVector) as the TOT revision
exit prematurely. But, I think the the data structures I'm using is very efficient: std::deque + std::map.

Shuxin


On 11/6/12 4:30 PM, Evan Cheng wrote:
> 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