[llvm-commits] [llvm] r170471 - in /llvm/trunk: lib/Transforms/InstCombine/InstCombineAddSub.cpp test/Transforms/InstCombine/fast-math.ll

[Shuxin Yang]

On 12/18/12 3:49 PM, Eli Friedman wrote:


>> +
>> +    union {
>> +      char FpValBuf[sizeof(APFloat)];
>> +      int dummy; // So this structure has at least 4-byte alignment.
>> +    };
> I'm sure I made a comment here before about using llvm::AlignedCharArrayUnion...
I'm sorry. I misunderstand your mail.
I thought you were saying, kind of: while the old-school code stink, it 
is kinda ok.
I will fix it.
>
> +
> +  // The outer loop works on one symbolic-value at a time. Suppose the input
> +  // addends are : <a1, x>, <b1, y>, <a2, x>, <c1, z>, <b2, y>, ...
> +  // The symbolic-values will be processed in this order: x, y, z.
> +  //
> +  for (unsigned SymIdx = 0; SymIdx < AddendNum; SymIdx++) {
> +
> +    const FAddend *ThisAddend = Addends[SymIdx];
> +    if (!ThisAddend) {
> +      // This addend was processed before.
> +      continue;
> +    }
> +
> +    Value *Val = ThisAddend->getSymVal();
> +    unsigned StartIdx = SimpVect.size();
> +    SimpVect.push_back(ThisAddend);
> +
> +    // The inner loop collects addends sharing same symbolic-value, and these
> +    // addends will be later on folded into a single addend. Following above
> +    // example, if the symbolic value "y" is being processed, the inner loop
> +    // will collect two addends "<b1,y>" and "<b2,Y>". These two addends will
> +    // be later on folded into "<b1+b2, y>".
> +    //
> +    for (unsigned SameSymIdx = SymIdx + 1;
> +         SameSymIdx < AddendNum; SameSymIdx++) {
> +      const FAddend *T = Addends[SameSymIdx];
> +      if (T && T->getSymVal() == Val) {
> +        // Set null such that next iteration of the outer loop will not process
> +        // this addend again.
> +        Addends[SameSymIdx] = 0;
> +        SimpVect.push_back(T);
> +      }
> +    }
> +
> +    // If multiple addends share same symbolic value, fold them together.
> +    if (StartIdx + 1 != SimpVect.size()) {
> +      FAddend &R = TmpResult[NextTmpIdx ++];
> +      R = *SimpVect[StartIdx];
> +      for (unsigned Idx = StartIdx + 1; Idx < SimpVect.size(); Idx++)
> +        R += *SimpVect[Idx];
> +
> +      // Pop all addends being folded and push the resulting folded addend.
> +      SimpVect.resize(StartIdx);
> +      if (Val != 0) {
> +        if (!R.isZero()) {
> +          SimpVect.push_back(&R);
> +        }
> +      } else {
> +        // Don't push constant addend at this time. It will be the last element
> +        // of <SimpVect>.
> +        ConstAdd = &R;
> +      }
> +    }
> +  }
> This loop seems overly complicated for what it's actually doing... are
> you sure you can't simplify it any?
If the comments are stripped, it would looks pretty clean.  It is bit 
hard to further simplify it.

>
>> +  assert((NextTmpIdx <= sizeof(TmpResult)/sizeof(TmpResult[0]) + 1) &&
>> +         "out-of-bound access");
> llvm::array_lengthof?
I will fix it.