Vector shuffling

On Sat, Sep 3, 2011 at 5:52 PM, Artem Shinkarov
<artyom.shinkaroff@gmail.com> wrote:
> On Fri, Sep 2, 2011 at 8:52 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
>> On Fri, 2 Sep 2011, Artem Shinkarov wrote:
>>
>>> Joseph, I don't understand this comment. I have 2 or 3 arguments in
>>> the VEC_SHUFFLE_EXPR and any of them can be C_MAYBE_CONST_EXPR,
>>
>> Yes.
>>
>>> so I
>>> need to wrap mask (the last argument) to avoid the following failure:
>>
>> No.  You need to fold it (c_fully_fold) to eliminate any
>> C_MAYBE_CONST_EXPR it contains, but you shouldn't need to wrap the result
>> of folding in a SAVE_EXPR.
>
> Ok, Now I get it, thanks.
>
> In the attachment there is a new version of the patch that removes save-exprs.

You are missing a ChangeLog entry

+
+res = __builtin_shuffle (a, mask1);       /* res is @{1,2,2,4@}  */
+res = __builtin_shuffle2 (a, b, mask2);   /* res is @{1,5,3,6@}  */

should be __builtin_shuffle (a, b, mask2);

+bool
+expand_vec_shuffle_expr_p (enum machine_mode mode, tree v0,
+                          tree v1, tree mask)
+{
+  int v0_mode_s = GET_MODE_BITSIZE (TYPE_MODE (TREE_TYPE (TREE_TYPE (v0))));
+  int mask_mode_s = GET_MODE_BITSIZE (TYPE_MODE (TREE_TYPE (TREE_TYPE
(mask))));
+
+  if (TREE_CODE (mask) == VECTOR_CST
+      && targetm.vectorize.builtin_vec_perm_ok (TREE_TYPE (v0), mask))
+    return true;
+
+  if (v0 != v1 || v0_mode_s != mask_mode_s)
+    return false;

the mask size check constrains the size of the vector elements but not
their count.  It looks like you should instead verify the vector modes
are equal?  At least that would match the fact that you have an expander
that distinguishes one operand mode only.

At some point we definitely want to merge the builtin_vec_perm_*
target hooks with the optab.

+      fn = copy_node (fn);

You shouldn't need to copy fn

+      call = fold_build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (fn)), fn);
+      call = build_call_nary (type /* ? */, call, 3, v0, v1, mask);
+
+      t = expand_normal (call);
+      target = gen_reg_rtx (mode);
+      emit_insn (gen_rtx_SET (VOIDmode, target, t));

why can't you simply use

  return expand_expr_real_1 (call, target, VOIDmode, EXPAND_NORMAL, NULL);

here?

+       case VEC_SHUFFLE_EXPR:
+         {
+           enum gimplify_status r0, r1, r2;
+
+           if (TREE_OPERAND (*expr_p, 0) == TREE_OPERAND (*expr_p, 1))
+             {
+               r0 = r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p,
+                                        post_p, is_gimple_val, fb_rvalue);
+               TREE_OPERAND (*expr_p, 1) = TREE_OPERAND (*expr_p, 0);
+             }
+           else
+             {
+                r0 = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p,
+                                    post_p, is_gimple_val, fb_rvalue);
+                r1 = gimplify_expr (&TREE_OPERAND (*expr_p, 1), pre_p,
+                                    post_p, is_gimple_val, fb_rvalue);
+             }

please avoid the above tree sharing (I realize it's probably for constants,
but we don't share those).  Thus, unconditionally gimplify both
operands.  The equality check in expanding should probably use
operand_equal_p (..., ..., 0) instead of a pointer comparison.

Thus, the above should simply use the expr_3: code, like FMA_EXPR.

+/* Vector shuffle expression. A = VEC_SHUFFLE_EXPR<v0, v1, maks>
+   means
+
+   freach i in length (mask):

foreach

+   number of elements in V0 and V1. The size of the inner type
+   of the MASK and of the V0 and V1 must be the same.

I leave the C frontend and x86 backend changes to the respective
maintainers.  Honza, can you have a look at the x86 changes?

Thanks,
Richard.

>
> Artem.
>

Vector shuffling

Commit Message

Patch