[llvm-commits] Patch: generalize vector shuffle

[Mon Ping Wang]

Hi Evan,

There are a few functions that duplicated between LegalizeTypes and  
LegalizeDAG.  I wasn't sure if the right thing to do here.  If  
LegalizeType will remain optional for sometime, I would have the two  
use the same function and have them shared (I don't think there is  
such a case today).   If LegalizeType was be required soon, I thought  
it would be easier to duplicate the two and delete it from LegalizeDAG  
as it wouldn't be necessary anymore.  However, it seems that I made a  
mistake of doing the 2nd way as this would cause some maintenance  
headaches until LegalizeType is required as well as making the patch  
more painful to digest.  I'll look for a place to house these  
utilities functions so they can be shared.  I'll clean up the early  
exit stuff and try to factor out more of SplitVecRes_VECTOR_SHUFFLE  
stuff.

Thanks,
-- Mon Ping



On Nov 2, 2008, at 11:47 PM, Evan Cheng wrote:

> Hi Mon Ping,
>
> Thanks for doing this. It's an important improvement.
>
> However, this patch is hard (at least for me) to digest. It's just a
> lot of code. :-) Some of the functions have become so big that it's
> hard to follow. For example, SplitVecRes_VECTOR_SHUFFLE. Is it
> possible to factor some stuff out?
>
> Also, some of code seems to be replicated in several functions (please
> correct me if that's not the case). For example, the code that
> determines how an element is used.
>
> A stylistic nitpick:
>
> +    if (VT1NumElems*2 == NumElems && SequentialMask(Mask, 0)) {
> +      setValue(&I, DAG.getNode(ISD::CONCAT_VECTORS, VT, V1, V2));
> +      return;
> +    }
> +    else {
>
> The else { } isn't needed because of the early exit. Same issue here:
>
> +    if (VT1NumElems == NumElems && SequentialMask(Mask,0)) {
> +      setValue(&I, V1);
> +      return;
> +    } else if (VT1NumElems == NumElems &&
> SequentialMask(Mask,NumElems)) {
> +      setValue(&I, V2);
> +      return;
> +    } else {
> +      // Analyze the access pattern of the vector to see if we can
> extract
> +      // two subvectors and do the shuffle.
>
> Don't forget to end comment sentences with periods. :-)
>
> Evan
>
> On Oct 31, 2008, at 7:56 PM, Mon Ping Wang wrote:
>
>> <genshuffle3.patch>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits