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

[Eli Friedman]

On Tue, Dec 18, 2012 at 4:09 PM, Shuxin Yang <shuxin.llvm at gmail.com> wrote:
> 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.

I wouldn't be quite so concerned about it if you actually got the
alignment correct... APFloat has a higher alignment than int on many
platforms.

>>
>> +
>> +  // 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.

It might end up being simpler if for each addend, you check all the
previous addends, rather than checking all the following addends.
Having two inner loops makes it more difficult to follow the code.

-Eli