[llvm-commits] Buggy SelectionDAG binop vector widening

[Mon Ping Wang]

Thanks for the patch.  Applied in r106038.

-- Mon Ping

On Jun 14, 2010, at 7:56 AM, Visa Putkinen wrote:

> Hi!
> 
> Sorry for the long delay.
> 
> On Wed, Jun 09, 2010 at 11:25:20PM -0700, Mon Ping Wang wrote:
>> All the occurrences in widening should use isTypeLegal should be updated to use isTypeSynthesizable but I can do that update if you prefer after this patch is in.
> 
> I replaced all isTypeLegals with isTypeSynthesizable in
> LegalizeVectorTypes.cpp to spare you the trouble.
> 
>> Looking at the changes, it overall looks good.  The code now separates the code that breaks the vector into synthesizable types with where it recombines it.  I think we can simplify the code during the recombine step
>>    while (true) {
>>      bool wrongTypeInConcatOps = false;
>>      for (Idx = ConcatEnd - 1; Idx >= 0; Idx--) {
>>        if (ConcatOps[Idx].getValueType() != MaxVT) {
>>          wrongTypeInConcatOps = true;
>>          break;
>>        }
>>      }
>>      if(!wrongTypeInConcatOps)
>>        break;
>>      ...
>>   }
>> 
>> When we split the vector into synthesizable types, we go form largest to smallest.  If the last piece is the list of operations we want to concatenate is MaxVT, I believe every other piece must be MaxVT so we don't need the loop. I think we can get rid of the for loop and we can move the condition into the while.  
> 
> A very good observation! I didn't like the clumsy for loop either, so
> good riddance. I implemented your suggestion and it does work: with the
> patch (against r105937) llc produces correct code from 
> 'fdiv <n x float> %a, %b' for x86-64 for vector lengths 1-32. The
> updated patch is attached.
> 
> Two DejaGNU test cases are also included in the patch.
> 
> -- 
> Visa Putkinen // vputkine at cs.hut.fi
> <vector-binop-widen.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits