On Mon, Jul 14, 2014 at 4:57 AM, Kugan <kugan.vivekanandara...@linaro.org> wrote: > On 11/07/14 22:47, Richard Biener wrote: >> On Fri, Jul 11, 2014 at 1:52 PM, Kugan >> <kugan.vivekanandara...@linaro.org> wrote: >>> Thanks foe the review and suggestions. >>> >>> On 10/07/14 22:15, Richard Biener wrote: >>>> On Mon, Jul 7, 2014 at 8:55 AM, Kugan <kugan.vivekanandara...@linaro.org> >>>> wrote: >>> >>> [...] >>> >>>>> >>>>> For -fwrapv, it is due to how PROMOTE_MODE is defined in arm back-end. >>>>> In the test-case, a function (which has signed char return type) returns >>>>> -1 in one of the paths. ARM PROMOTE_MODE changes that to 255 and relies >>>>> on zero/sign extension generated by RTL again for the correct value. I >>>>> saw some other targets also defining similar think. I am therefore >>>>> skipping removing zero/sign extension if the ssa variable can be set to >>>>> negative integer constants. >>>> >>>> Hm? I think you should rather check that you are removing a >>>> sign-/zero-extension - PROMOTE_MODE tells you if it will sign- or >>>> zero-extend. Definitely >>>> >>>> + /* In some architectures, negative integer constants are truncated and >>>> + sign changed with target defined PROMOTE_MODE macro. This will impact >>>> + the value range seen here and produce wrong code if zero/sign >>>> extensions >>>> + are eliminated. Therefore, return false if this SSA can have negative >>>> + integers. */ >>>> + if (is_gimple_assign (stmt) >>>> + && (TREE_CODE_CLASS (gimple_assign_rhs_code (stmt)) == tcc_unary)) >>>> + { >>>> + tree rhs1 = gimple_assign_rhs1 (stmt); >>>> + if (TREE_CODE (rhs1) == INTEGER_CST >>>> + && !TYPE_UNSIGNED (TREE_TYPE (ssa)) >>>> + && tree_int_cst_compare (rhs1, integer_zero_node) == -1) >>>> + return false; >>>> >>>> looks completely bogus ... (an unary op with a constant operand?) >>>> instead you want to do sth like >>> >>> I see that unary op with a constant operand is not possible in gimple. >>> What I wanted to check here is any sort of constant loads; but seems >>> that will not happen in gimple. Is PHI statements the only possible >>> statements where we will end up with such constants. >> >> No, in theory you can have >> >> ssa_1 = -1; >> >> but that's not unary but a GIMPLE_SINGLE_RHS and thus >> gimple_assign_rhs_code (stmt) == INTEGER_CST. >> >>>> mode = TYPE_MODE (TREE_TYPE (ssa)); >>>> rhs_uns = TYPE_UNSIGNED (TREE_TYPE (ssa)); >>>> PROMOTE_MODE (mode, rhs_uns, TREE_TYPE (ssa)); >>>> >>>> instead of initializing rhs_uns from ssas type. That is, if >>>> PROMOTE_MODE tells you to promote _not_ according to ssas sign then >>>> honor that. >>> >>> This is triggered in pr43017.c in function foo for arm-none-linux-gnueabi. >>> >>> where, the gimple statement that cause this looks like: >>> ..... >>> # _3 = PHI <_17(7), -1(2)> >>> bb43: >>> return _3; >>> >>> ARM PROMOTE_MODE changes the sign for integer constants only and hence >>> looking at the variable with PROMOTE_MODE is not changing the sign in >>> this case. >>> >>> #define PROMOTE_MODE(MODE, UNSIGNEDP, TYPE) \ >>> if (GET_MODE_CLASS (MODE) == MODE_INT \ >>> && GET_MODE_SIZE (MODE) < 4) \ >>> { \ >>> if (MODE == QImode) \ >>> UNSIGNEDP = 1; \ >>> else if (MODE == HImode) \ >>> UNSIGNEDP = 1; \ >>> (MODE) = SImode; \ >>> } >> >> Where does it only apply for "constants"? It applies to all QImode and >> HImode entities. > > oops, sorry. I don’t know what I was thinking or looking at when I wrote > that :( It indeed fixes my problems. Thanks for that. > > Here is the modified patch. Bootstrapped and regression tested for > 86_64-unknown-linux-gnu and arm-none-linux-gnueabi with no new regressions. > > > Is this OK?
+ lhs_type = lang_hooks.types.type_for_mode (lhs_mode, lhs_uns); ... + && ((!lhs_uns && !wi::neg_p (min, TYPE_SIGN (lhs_type))) ... + type_min = wide_int::from (TYPE_MIN_VALUE (lhs_type), prec, + TYPE_SIGN (TREE_TYPE (ssa))); + type_max = wide_int::from (TYPE_MAX_VALUE (lhs_type), prec, + TYPE_SIGN (TREE_TYPE (ssa))); you shouldn't try getting at lhs_type. Btw, do you want to constrain lhs_mode to MODE_INTs somewhere? For TYPE_SIGN use lhs_uns instead, for the min/max value you should use wi::min_value () and wi::max_value () instead. You are still using TYPE_SIGN (TREE_TYPE (ssa)) here and later, but we computed rhs_uns "properly" using PROMOTE_MODE. I think the code with re-setting lhs_uns if rhs_uns != lhs_uns and later using TYPE_SIGN again is pretty hard to follow. Btw, it seems you need to conditionalize the call to PROMOTE_MODE on its availability. Isn't it simply about choosing a proper range we need to restrict ssa to? That is, dependent on rhs_uns computed by PROMOTE_MODE, simply: + mode = TYPE_MODE (TREE_TYPE (ssa)); + rhs_uns = TYPE_UNSIGNED (TREE_TYPE (ssa)); #ifdef PROMOTE_MODE + PROMOTE_MODE (mode, rhs_uns, TREE_TYPE (ssa)); #endif if (rhs_uns) return wi::ge_p (min, 0); // if min >= 0 then range contains positive values else return wi::le_p (max, wi::max_value (TYPE_PRECISION (TREE_TYPE (ssa)), SIGNED); // if max <= signed-max-of-type then range doesn't need sign-extension ? Thanks, Richard. > Thanks, > Kugan > > > gcc/ > > 2014-07-14 Kugan Vivekanandarajah <kug...@linaro.org> > > * calls.c (precompute_arguments): Check is_promoted_for_type > and set the promoted mode. > (is_promoted_for_type): New function. > (expand_expr_real_1): Check is_promoted_for_type > and set the promoted mode. > * expr.h (is_promoted_for_type): New function definition. > * cfgexpand.c (expand_gimple_stmt_1): Call emit_move_insn if > SUBREG is promoted with SRP_SIGNED_AND_UNSIGNED. > > > gcc/testsuite > 2014-07-14 Kugan Vivekanandarajah <kug...@linaro.org> > > * gcc.dg/zero_sign_ext_test.c: New test.