On Wed, Oct 23, 2013 at 11:18 PM, Jakub Jelinek <ja...@redhat.com> wrote: > On Wed, Oct 23, 2013 at 09:40:21PM -0700, Cong Hou wrote: >> On Wed, Oct 23, 2013 at 8:52 AM, Joseph S. Myers >> <jos...@codesourcery.com> wrote: >> > On Tue, 22 Oct 2013, Cong Hou wrote: >> > >> >> For abs(char/short), type conversions are needed as the current abs() >> >> function/operation does not accept argument of char/short type. >> >> Therefore when we want to get the absolute value of a char_val using >> >> abs (char_val), it will be converted into abs ((int) char_val). It >> >> then can be vectorized, but the generated code is not efficient as >> >> lots of packings and unpackings are envolved. But if we convert >> >> (char) abs ((int) char_val) to abs (char_val), the vectorizer will be >> >> able to generate better code. Same for short. >> > >> > ABS_EXPR has undefined overflow behavior. Thus, abs ((int) -128) is >> > defined (and we also define the subsequent conversion of +128 to signed >> > char, which ISO C makes implementation-defined not undefined), and >> > converting to an ABS_EXPR on char would wrongly make it undefined. For >> > such a transformation to be valid (in the absence of VRP saying that -128 >> > isn't a possible value) you'd need a GIMPLE representation for >> > ABS_EXPR<overflow:wrap>, as distinct from ABS_EXPR<overflow:undefined>. >> > You don't have the option there is for some arithmetic operations of >> > converting to a corresponding operation on unsigned types. >> > >> >> Yes, you are right. The method I use can guarantee wrapping on >> overflow (either shift-xor-sub or max(x, -x)). Can I just add the >> condition if (flag_wrapv) before the conversion I made to prevent the >> undefined behavior on overflow? > > What HW insns you expand to is one thing, but if some GCC pass assumes that > ABS_EXPR always returns non-negative value (many do, look e.g. at > tree_unary_nonnegative_warnv_p, extract_range_from_unary_expr_1, > simplify_const_relational_operation, etc., you'd need to grep for all > ABS_EXPR/ABS occurrences) and optimizes code based on that fact, you get > wrong code because (char) abs((char) -128) is well defined. > If we change ABS_EXPR/ABS definition that it is well defined on the most > negative value of the typ (resp. mode), then we loose all those > optimizations, if we do that only for the char/short types, it would be > quite weird, though we could keep the benefits, but at the RTL level we'd > need to treat that way all the modes equal to short's mode and smaller (so, > for sizeof(short) == sizeof(int) target even int's mode).
I checked those functions and they all consider the possibility of overflow. For example, tree_unary_nonnegative_warnv_p only returns true for ABS_EXPR on integers if overflow is undefined. If the consequence of overflow is wrapping, I think converting (char) abs((int)-128) to abs(-128) (-128 has char type) is safe. Can we do it by checking flag_wrapv? I could also first remove the abs conversion content from this patch but only keep the content of expanding abs() for i386. I will submit it later. > > The other possibility is not to create the ABS_EXPRs of char/short anywhere, > solve the vectorization issues either through tree-vect-patterns.c or > as part of the vectorization type demotion/promotions, see the recent > discussions for that, you'd represent the short/char abs for the vectorized > loop say using the shift-xor-sub or builtin etc. and if you want to do the > same thing for scalar code, you'd just have combiner try to match some > sequence. Yes, I could do it through tree-vect-patterns.c, if the abs conversion is prohibited. Currently the only reason I need the abs conversion is for vectorization. Vectorization type demotion/promotions is interesting, but I am afraid we will face the same problem there. Thank you for your comment! Cong > > Jakub
