[Bug target/52607] v4df __builtin_shuffle with {0,2,1,3} or {1,3,0,2}

["marc.glisse at normalesup dot org" <gcc-bugzilla at gcc dot gnu dot org>]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52607

--- Comment #24 from Marc Glisse <marc.glisse at normalesup dot org> 2012-03-29 14:19:11 UTC ---
(In reply to comment #23)
> (In reply to comment #18)
> 
> +  if (!d->testing_p)
> +    dsecond.target = gen_reg_rtx (dsecond.vmode);
> +  dfirst.op1 = dsecond.target;
> 
> This bit has a problem with testing_p in that we'll have op0==op1
> while testing and not when expanding.  Which means that testing_p
> will be checking something else.

Unless d->target==d->op0 (is that the case? I was kind of assuming it wasn't),
it looks ok, but I agree that it should be improved. From other code, it looks
like using gen_reg_rtx in testing is fine and avoiding it is just an
optimization.

On the other hand, if I remember correctly, the function could just return true
early when testing (like the other function does) and assert during expansion,
since it is not supposed to fail (except for the initial mode/target check),
that would document the intent better.

> I've been meaning to convert i386 from op0==op1 to one_operand_p,
> like I used in targets I converted later, like ia64.  I'll see about
> making this change this afternoon, and then you can update your
> patch to match.

ok (no promise timewise).

> +  ok = expand_vec_perm_1 (&dsecond);
> +  ok &= ix86_expand_vec_perm_const_1 (&dfirst);
> +
> +  if (!ok)
> +    return false;
> +
> +  return true;
> 
> Better with a short-circuit to avoid extra work:
> 
>   return (expand_vec_perm_1 (&dsecond)
>           && ix86_expand_vec_perm_const_1 (&dfirst));

Indeed!

Thanks for the comments.