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

[Eli Friedman]

+    SmallVector<int, 32> Mask(32U, -1);
     cast<ShuffleVectorSDNode>(Node)->getMask(Mask);

What's the point of initializing the vector when it gets rebuilt in
the next line?

+      if (NewEltVT.bitsLT(EltVT)) {

It might be worth a short comment noting that BUILD_VECTOR operands
are allowed to be wider than the element type.

+        // Convert shuffle node.
+        // If original node was v4i64 and the new EltVT is i32,
+        // cast operands to v8i32 and re-build the mask.

"For example, if the original node was..."

Please commit with those changes.

-Eli

On Tue, Jan 3, 2012 at 1:06 AM, Demikhovsky, Elena
<elena.demikhovsky at intel.com> wrote:
> Eli,
> 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.