[llvm-commits] [llvm] r147399 - in /llvm/trunk: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp test/CodeGen/X86/avx-shuffle-x86_32.ll Please review

[Demikhovsky, Elena]

This is the original code that converts VECTOR_SHUFFLE that can be lowered to BUILD_VECTOR.
   
 if (!TLI.isTypeLegal(EltVT)) {

      EVT EltVT = TLI.getTypeToTransformTo(*DAG.getContext(), EltVT); 
}
The code failed with assertion on Win32 because "i64" was transfromed to "i32" and BUILD_VECTOR has illegal form.

In the first patch I assumed that the legal type is always smaller than original and this assumption was wrong.

In this patch I check that "i64" > "i32" and then rebuild "shuffle" before transforming it to BUILD_VECTOR.
The "rebuild" means that I convert v4i64 shuffle operation to v8i32 and convert the mask.
If the original mask was
5          -1         2       4
the new mask will be
10, 11,  -1,  -1,  4, 5,  8, 9

- Elena

-----Original Message-----
From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Duncan Sands
Sent: Tuesday, January 03, 2012 11:13
To: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] [llvm] r147399 - in /llvm/trunk: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp test/CodeGen/X86/avx-shuffle-x86_32.ll Please review

Hi Elena, what about his other comments?  In particular I too would have liked
to know what was wrong in the first version, i.e. what change "I fixed the
patch" was referring to.

Ciao, Duncan.

> You are right about the mask. I fixed it.
> Please look again.
>
>
> - Elena
>
> -----Original Message-----
> From: Eli Friedman [mailto:eli.friedman at gmail.com]
> Sent: Monday, January 02, 2012 19:57
> To: Demikhovsky, Elena
> Cc: Rafael Ávila de Espíndola; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [llvm] r147399 - in /llvm/trunk: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp test/CodeGen/X86/avx-shuffle-x86_32.ll Please review
>
> 2012/1/2 Demikhovsky, Elena<elena.demikhovsky at intel.com>:
>> I fixed the patch and want to commit it.
>> I re-checked all and CodeGen/ARM/vext.ll passes now.
>>
>> Please review.
>
> What changed to fix this patch?
>
> For the following:
> +        // EltVT gets smaller
> +        assert(factor>  0);
> +        SmallVector<int, 32>  NewMask(32U, -1);
>
> Where does the "32U" come from in "NewMask(32U, -1)"?
>
> For the following:
> +        for (unsigned i = 0; i<  VT.getVectorNumElements(); ++i) {
> +          if (Mask[i]<  0) {
> +            for (unsigned fi = 0; fi<  factor; ++fi)
> +              NewMask[i*factor+fi] = Mask[i];
> +          }
> +          else {
> +            for (unsigned fi = 0; fi<  factor; ++fi)
> +              NewMask[i*factor+fi] = Mask[i]*factor+fi;
> +          }
> +          Mask = NewMask;
> +        }
>
> The assignment Mask = NewMask inside the loop looks very suspicious:
> are you sure that's correct?
>
> -Eli
> ---------------------------------------------------------------------
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.