[PATCH] Fix vector rotate regression (PR tree-optimization/57233)

[Jakub Jelinek]

On Thu, Jun 26, 2014 at 07:43:55AM +0200, Marc Glisse wrote:
> >+	  if (compute_type == TREE_TYPE (type)
> >+	      && !VECTOR_INTEGER_TYPE_P (TREE_TYPE (rhs2)))
> >+	    {
> >+	      optab oplv, opl, oprv, opr, opo;
> >+	      oplv = optab_for_tree_code (LSHIFT_EXPR, type, optab_vector);
> >+	      oprv = optab_for_tree_code (RSHIFT_EXPR, type, optab_vector);
> >+	      opo = optab_for_tree_code (BIT_IOR_EXPR, type, optab_default);
> >+	      opl = optab_for_tree_code (LSHIFT_EXPR, type, optab_scalar);
> >+	      opr = optab_for_tree_code (RSHIFT_EXPR, type, optab_scalar);
> 
> How well are we separating lshiftrt from ashiftrt? Are ROTATE_EXPR always on
> an unsigned type so it is fine? Or do we expand one in terms of the other if
> it isn't available so it doesn't matter?

You're right, for RSHIFT_EXPR I should use
	oprv = vlshr_optab;
	opr = lshr_optab;
because expmed.c always uses lshiftrt.
> 
> >+	      if (optab_handler (oplv, TYPE_MODE (type)) != CODE_FOR_nothing
> >+		  && optab_handler (opl, TYPE_MODE (type)) == CODE_FOR_nothing
> >+		  && optab_handler (oprv, TYPE_MODE (type)) != CODE_FOR_nothing
> >+		  && optab_handler (opr, TYPE_MODE (type)) == CODE_FOR_nothing)
> >+		{
> >+		  opl = oplv;
> >+		  opr = oprv;
> >+		}
> 
> Maybe arrange the conditions in order (oplv!= && oprv!= && (opl== ||
> opr==)), or separate the replacement of opl and of opv into 2 separate ifs?

The reason I wrote them in this order was so that it fits on the lines ;)
Guess two separate ifs would be fine.

> Somehow, it feels like those checks should be somewhere in get_compute_type
> so we test both scalar and vector versions for each size, or we could call
> get_compute_type for both and pick the best.

It would be easier to just call get_compute_type in each case and pick the
wider, but the preexisting case didn't bother, so I haven't bothered either.
Passing two optabs optionally to get_compute_type would be IMHO too ugly.
> 
> >+	      compute_type = get_compute_type (LSHIFT_EXPR, opl, type);
> >+	      if (compute_type == TREE_TYPE (type)
> >+		  || compute_type != get_compute_type (RSHIFT_EXPR, opr, type)
> >+		  || compute_type != get_compute_type (BIT_IOR_EXPR, opo, type))
> >+		compute_type = TREE_TYPE (type);
> 
> Since we have determined compute_type from ashift (let's assume that's the
> one least likely to exist), I would just check that optab is ok with using
> this mode for the other 2 ops. Here, if we have shifts in 128 bits and ior
> in both 128 and 256 bits, we will fail (I thought that might be the case in
> AVX, but apparently not). Plus it is faster ;-)

Makes sense.

> Does rotate hit PR 56873? (I noticed the -mno-xop and no -mxop test)

There are exec testcases, -mxop AFAIK has rotates, so I think it is not
worth testing.  And there is a single asm match compile testcase, where
the -mno-xop ensures that when somebody -mxop in RUNTESTFLAGS or CPU
defaults to -mxop we don't get a matching failure, because in that case it
would emit a vector rotate insn.

	Jakub