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

[Shuxin Yang]

You are actually quite right on the alignment thing.  APFloat actually 
has more stringent alignment, and I should not hard-coded
the alignment in the code.

Inserting a dummy field was my experiment in order to move on. 
Unfortunately I forget  going back to make sure it works.
Sorry for the sloppiness.


On 12/18/12 4:24 PM, Eli Friedman wrote:
> 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