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
