[llvm] r178793 - Reassociate: Avoid iterator invalidation.

[Benjamin Kramer]

On 05.04.2013, at 05:19, Shuxin Yang <shuxin.llvm at gmail.com> wrote:

> Thank you for fixing this bug.
> 
> Alternatively, we can set the capacity of the vector at very beginning. This way we are still able to
> access the operand through pointer, instead of relatively more expensive "vector[other-vector[idx]".

It's not that expensive, just an additional add, potentially in an addressing mode. If you make the vector constant sized you should add some comments so other people know that they can't add more things to the vector.

> Can I get the big testing case offline?

Sure, I attached it. I got invalid reads with valgrind. asan probably works too.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ntpd-oncore-testcase.ll
Type: application/octet-stream
Size: 8371 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130405/18abc3ac/attachment.obj>
-------------- next part --------------


- Ben
> 
> Thanks
> Shuxin
> 
> 
> 
> On 4/4/13 2:15 PM, Benjamin Kramer wrote:
>> Author: d0k
>> Date: Thu Apr  4 16:15:42 2013
>> New Revision: 178793
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=178793&view=rev
>> Log:
>> Reassociate: Avoid iterator invalidation.
>> 
>> OpndPtrs stored pointers into the Opnd vector that became invalid when the
>> vector grows. Store indices instead. Sadly I only have a large testcase that
>> only triggers under valgrind, so I didn't include it.
>> 
>> Modified:
>>     llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp
>> 
>> Modified: llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp?rev=178793&r1=178792&r2=178793&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp Thu Apr  4 16:15:42 2013
>> @@ -143,9 +143,13 @@ namespace {
>>      //   So, if Rank(X) < Rank(Y) < Rank(Z), it means X is defined earlier
>>      //   than Y which is defined earlier than Z. Permute "x | 1", "Y & 2",
>>      //   "z" in the order of X-Y-Z is better than any other orders.
>> -    struct PtrSortFunctor {
>> -      bool operator()(XorOpnd * const &LHS, XorOpnd * const &RHS) {
>> -        return LHS->getSymbolicRank() < RHS->getSymbolicRank();
>> +    class PtrSortFunctor {
>> +      ArrayRef<XorOpnd> A;
>> +
>> +    public:
>> +      PtrSortFunctor(ArrayRef<XorOpnd> Array) : A(Array) {}
>> +      bool operator()(unsigned LHSIndex, unsigned RHSIndex) {
>> +        return A[LHSIndex].getSymbolicRank() < A[RHSIndex].getSymbolicRank();
>>        }
>>      };
>>    private:
>> @@ -1270,7 +1274,7 @@ Value *Reassociate::OptimizeXor(Instruct
>>      return 0;
>>      SmallVector<XorOpnd, 8> Opnds;
>> -  SmallVector<XorOpnd*, 8> OpndPtrs;
>> +  SmallVector<unsigned, 8> OpndIndices;
>>    Type *Ty = Ops[0].Op->getType();
>>    APInt ConstOpnd(Ty->getIntegerBitWidth(), 0);
>>  @@ -1281,7 +1285,7 @@ Value *Reassociate::OptimizeXor(Instruct
>>        XorOpnd O(V);
>>        O.setSymbolicRank(getRank(O.getSymbolicPart()));
>>        Opnds.push_back(O);
>> -      OpndPtrs.push_back(&Opnds.back());
>> +      OpndIndices.push_back(Opnds.size() - 1);
>>      } else
>>        ConstOpnd ^= cast<ConstantInt>(V)->getValue();
>>    }
>> @@ -1290,13 +1294,14 @@ Value *Reassociate::OptimizeXor(Instruct
>>    //  the same symbolic value cluster together. For instance, the input operand
>>    //  sequence ("x | 123", "y & 456", "x & 789") will be sorted into:
>>    //  ("x | 123", "x & 789", "y & 456").
>> -  std::sort(OpndPtrs.begin(), OpndPtrs.end(), XorOpnd::PtrSortFunctor());
>> +  std::sort(OpndIndices.begin(), OpndIndices.end(),
>> +            XorOpnd::PtrSortFunctor(Opnds));
>>      // Step 3: Combine adjacent operands
>>    XorOpnd *PrevOpnd = 0;
>>    bool Changed = false;
>>    for (unsigned i = 0, e = Opnds.size(); i < e; i++) {
>> -    XorOpnd *CurrOpnd = OpndPtrs[i];
>> +    XorOpnd *CurrOpnd = &Opnds[OpndIndices[i]];
>>      // The combined value
>>      Value *CV;
>>  
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>