| Message ID | 53C34734.2080103@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 14 July 2014 04:58:17 Kugan <kugan.vivekanandarajah@linaro.org> wrote: > On 11/07/14 22:47, Richard Biener wrote: > > On Fri, Jul 11, 2014 at 1:52 PM, Kugan > > <kugan.vivekanandarajah@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.vivekanandarajah@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? > > Thanks, > Kugan > > > gcc/ > > 2014-07-14 Kugan Vivekanandarajah <kuganv@linaro.org> > > * calls.c (precompute_arguments): Check is_promoted_for_type > and set the promoted mode. > (is_promoted_for_type): New function. Don't we name predicates more like promoted_for_type_p? Thanks, > (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 <kuganv@linaro.org> > > * gcc.dg/zero_sign_ext_test.c: New test. Sent with AquaMail for Android http://www.aqua-mail.com
On Mon, Jul 14, 2014 at 4:57 AM, Kugan <kugan.vivekanandarajah@linaro.org> wrote: > On 11/07/14 22:47, Richard Biener wrote: >> On Fri, Jul 11, 2014 at 1:52 PM, Kugan >> <kugan.vivekanandarajah@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.vivekanandarajah@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 <kuganv@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 <kuganv@linaro.org> > > * gcc.dg/zero_sign_ext_test.c: New test.
> + 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? Is this in addition to !INTEGRAL_TYPE_P (TREE_TYPE (ssa)))? Do you mean that I should check lhs_mode as well? > 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 I think we will have to check that ssa has necessary sign/zero extension when assigned to lhs_type. If PROMOTE_MODE tells us that ssa's type will be interpreted differently, the value range of ssa also will have corresponding range. In this cases, shouldn’t we have to check for upper and lower limit for both min and max? How about this? bool promoted_for_type_p (tree ssa, enum machine_mode lhs_mode, bool lhs_uns) { wide_int min, max; tree lhs_type, rhs_type; bool rhs_uns; enum machine_mode rhs_mode; tree min_tree, max_tree; if (ssa == NULL_TREE || TREE_CODE (ssa) != SSA_NAME || !INTEGRAL_TYPE_P (TREE_TYPE (ssa))) return false; /* Return FALSE if value_range is not recorded for SSA. */ if (get_range_info (ssa, &min, &max) != VR_RANGE) return false; rhs_uns = TYPE_UNSIGNED (TREE_TYPE (ssa)); if (rhs_uns != lhs_uns) { /* Signedness of LHS and RHS differs and values also cannot be represented in LHS range. */ unsigned int prec = min.get_precision (); if ((lhs_uns && wi::neg_p (min, rhs_uns ? UNSIGNED : SIGNED)) || (!lhs_uns && !wi::le_p (max, wi::max_value (prec, SIGNED), rhs_uns ? UNSIGNED : SIGNED))) return false; } /* In some architectures, modes are promoted and sign changed with target defined PROMOTE_MODE macro. If PROMOTE_MODE tells you to promote _not_ according to ssa's sign then honour that. */ rhs_mode = TYPE_MODE (TREE_TYPE (ssa)); #ifdef PROMOTE_MODE PROMOTE_MODE (rhs_mode, rhs_uns, TREE_TYPE (ssa)); #endif rhs_type = lang_hooks.types.type_for_mode (rhs_mode, rhs_uns); lhs_type = lang_hooks.types.type_for_mode (lhs_mode, lhs_uns); min_tree = wide_int_to_tree (rhs_type, min); max_tree = wide_int_to_tree (rhs_type, max); /* Check if values lies in-between the type range. */ if (int_fits_type_p (min_tree, lhs_type) && int_fits_type_p (max_tree, lhs_type)) return true; else return false; } Thanks, Kugan
On Fri, Aug 1, 2014 at 6:51 AM, Kugan <kugan.vivekanandarajah@linaro.org> wrote: >> + 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? > > Is this in addition to !INTEGRAL_TYPE_P (TREE_TYPE (ssa)))? Do you mean > that I should check lhs_mode as well? No, that's probably enough. >> 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 > > I think we will have to check that ssa has necessary sign/zero extension > when assigned to lhs_type. If PROMOTE_MODE tells us that ssa's type will > be interpreted differently, the value range of ssa also will have > corresponding range. In this cases, shouldn’t we have to check for > upper and lower limit for both min and max? Hmm? That's exactly what the check is testing... we know that min <= max thus if min >= 0 then max >= 0. zero_extension will never do anything on [0, INF] If max < MAX-SIGNED then sign-extension will not do anything. Ok, sign-extension will do sth for negative values still. So rather if (rhs_uns) return wi::geu_p (min, 0); else return wi::ges_p (min, 0) && wi::les_p (max, wi::max_value (TYPE_PRECISION (TREE_TYPE (ssa)), SIGNED)); ? I don't like the use of int_fits_type_p you propose. Richard. > How about this? > > bool > promoted_for_type_p (tree ssa, enum machine_mode lhs_mode, bool lhs_uns) > { > wide_int min, max; > tree lhs_type, rhs_type; > bool rhs_uns; > enum machine_mode rhs_mode; > tree min_tree, max_tree; > > if (ssa == NULL_TREE > || TREE_CODE (ssa) != SSA_NAME > || !INTEGRAL_TYPE_P (TREE_TYPE (ssa))) > return false; > > /* Return FALSE if value_range is not recorded for SSA. */ > if (get_range_info (ssa, &min, &max) != VR_RANGE) > return false; > > rhs_uns = TYPE_UNSIGNED (TREE_TYPE (ssa)); > if (rhs_uns != lhs_uns) > { > /* Signedness of LHS and RHS differs and values also cannot be > represented in LHS range. */ > unsigned int prec = min.get_precision (); > if ((lhs_uns && wi::neg_p (min, rhs_uns ? UNSIGNED : SIGNED)) > || (!lhs_uns && !wi::le_p (max, > wi::max_value (prec, SIGNED), > rhs_uns ? UNSIGNED : SIGNED))) > return false; > } > > /* In some architectures, modes are promoted and sign changed with > target defined PROMOTE_MODE macro. If PROMOTE_MODE tells you to > promote _not_ according to ssa's sign then honour that. */ > rhs_mode = TYPE_MODE (TREE_TYPE (ssa)); > #ifdef PROMOTE_MODE > PROMOTE_MODE (rhs_mode, rhs_uns, TREE_TYPE (ssa)); > #endif > > rhs_type = lang_hooks.types.type_for_mode (rhs_mode, rhs_uns); > lhs_type = lang_hooks.types.type_for_mode (lhs_mode, lhs_uns); > min_tree = wide_int_to_tree (rhs_type, min); > max_tree = wide_int_to_tree (rhs_type, max); > > /* Check if values lies in-between the type range. */ > if (int_fits_type_p (min_tree, lhs_type) > && int_fits_type_p (max_tree, lhs_type)) > return true; > else > return false; > } > > > Thanks, > Kugan
>>> 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 >> >> I think we will have to check that ssa has necessary sign/zero extension >> when assigned to lhs_type. If PROMOTE_MODE tells us that ssa's type will >> be interpreted differently, the value range of ssa also will have >> corresponding range. In this cases, shouldn’t we have to check for >> upper and lower limit for both min and max? > > Hmm? That's exactly what the check is testing... we know that > min <= max thus if min >= 0 then max >= 0. > > zero_extension will never do anything on [0, INF] > > If max < MAX-SIGNED then sign-extension will not do anything. Ok, > sign-extension will do sth for negative values still. So rather > > if (rhs_uns) > return wi::geu_p (min, 0); > else > return wi::ges_p (min, 0) && wi::les_p (max, wi::max_value > (TYPE_PRECISION (TREE_TYPE (ssa)), SIGNED)); > > ? Thanks for the explanation. I agree. Don’t we have to however check this on lhs_uns as this function is checking if ssa is promoted for lhs_sign and lhs_mode? Here is an attempt based on this. I ran regression testing with arm-none-linux-gnueabi on qemu-arm without any new regressions. Sine I am not comparing value ranges to see if it can be represented in lhs_sigh, I can now skip the PROMOTED_MODE check. I am still using wide_int::from (instead of wi::max_value) to get the limit as I have to match the precision with min, max precision. otherwise wide_int comparisons will not work. Is there a better way for this? /* Return TRUE if value in SSA is already zero/sign extended for lhs type (type here is the combination of LHS_MODE and LHS_UNS) using value range information stored. Return FALSE otherwise. */ bool promoted_for_type_p (tree ssa, enum machine_mode lhs_mode, bool lhs_uns) { wide_int min, max, limit; tree lhs_type; bool rhs_uns; signop rhs_signop; if (ssa == NULL_TREE || TREE_CODE (ssa) != SSA_NAME || !INTEGRAL_TYPE_P (TREE_TYPE (ssa))) return false; /* Return FALSE if value_range is not recorded for SSA. */ if (get_range_info (ssa, &min, &max) != VR_RANGE) return false; rhs_uns = TYPE_UNSIGNED (TREE_TYPE (ssa)); rhs_signop = rhs_uns ? UNSIGNED : SIGNED; lhs_type = lang_hooks.types.type_for_mode (lhs_mode, lhs_uns); limit = wide_int::from (TYPE_MAX_VALUE (lhs_type), TYPE_PRECISION (TREE_TYPE (ssa)), SIGNED); if (lhs_uns) /* If min >= 0 then range contains positive values and doesnt need zero-extension. */ return wi::ge_p (min, 0, rhs_signop); else /* If min >= 0 and max <= signed-max-of-type then range doesn't need sign-extension. */ return wi::ge_p (min, 0, rhs_signop) && wi::le_p (max, limit, rhs_signop); } Thanks, Kugan
On 02/08/14 02:03, Kugan wrote: >>>> 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 >>> >>> I think we will have to check that ssa has necessary sign/zero extension >>> when assigned to lhs_type. If PROMOTE_MODE tells us that ssa's type will >>> be interpreted differently, the value range of ssa also will have >>> corresponding range. In this cases, shouldn’t we have to check for >>> upper and lower limit for both min and max? >> >> Hmm? That's exactly what the check is testing... we know that >> min <= max thus if min >= 0 then max >= 0. >> >> zero_extension will never do anything on [0, INF] >> >> If max < MAX-SIGNED then sign-extension will not do anything. Ok, >> sign-extension will do sth for negative values still. So rather >> >> if (rhs_uns) >> return wi::geu_p (min, 0); >> else >> return wi::ges_p (min, 0) && wi::les_p (max, wi::max_value >> (TYPE_PRECISION (TREE_TYPE (ssa)), SIGNED)); >> >> ? Looking at your comments again, I think we have to consider three things here. To be able assign to LHS (of lhs_uns and lhs_mode) without conversion of RHS (tree SSA) * If we ignore the mode changes (i.e. LHS_mode can be different in terms of precision) and ignore PROMOTE_MODE and consider only the sign of LHS and RHS if (lhs_uns) return wi::ge_p (min, 0, rhs_signop); // if min >= 0 then range contains positive values else if (rhs_uns) // if max <= signed-max-of-type then range doesn't need sign-extension return wi::le_p (max, wi::max_value (TYPE_PRECISION (TREE_TYPE (ssa)), SIGNED); else return true; * However, if we consider the PROMOTE_MODE might change the RHS sign if (lhs_uns) { return wi::ge_p (min, 0, rhs_signop); } else { signed_max = wide_int::from (TYPE_MAX_VALUE (lhs_type), TYPE_PRECISION (TREE_TYPE (ssa)), rhs_signop); if (rhs_uns) /* If PROMOTE_MODE changed an RHS signed to unsigned and SSA contains negative value range, we still have to do sign-extend. */ return wi::ge_p (min, 0, TYPE_SIGN (TREE_TYPE (ssa))) && wi::le_p (max, signed_max, rhs_signop); else /* If PROMOTE_MODE changed an RHS unsigned to signed and SSA contains value range more than signed-max-of-type, we still have to do sign-extend. */ return wi::le_p (max, signed_max, TYPE_SIGN (TREE_TYPE (ssa))); } * If we also consider that LHS mode and RHS mode precision can be different if (lhs_uns) { unsigned_max = wide_int::from (TYPE_MAX_VALUE (lhs_type), TYPE_PRECISION (TREE_TYPE (ssa)), rhs_signop); /* If min >= 0 then range contains positive values and doesnt need zero-extension. If max <= unsigned-max-of-type, then value fits type. */ return wi::ge_p (min, 0, rhs_signop) && wi::le_p (max, unsigned_max, rhs_signop); } else { signed_max = wide_int::from (TYPE_MAX_VALUE (lhs_type), TYPE_PRECISION (TREE_TYPE (ssa)), rhs_signop); signed_min = wide_int::from (TYPE_MIN_VALUE (lhs_type), TYPE_PRECISION (TREE_TYPE (ssa)), rhs_signop); if (rhs_uns) /* If PROMOTE_MODE changed an RHS signed to unsigned and SSA contains negative value range, we still have to do sign-extend. */ return wi::ge_p (min, 0, TYPE_SIGN (TREE_TYPE (ssa))) && wi::le_p (max, signed_max, rhs_signop); else /* If PROMOTE_MODE changed an RHS unsigned to signed and SSA contains value range more than signed-max-of-type, we still have to do sign-extend. */ return wi::le_p (max, signed_max, TYPE_SIGN (TREE_TYPE (ssa))) && wi::ge_p (min, signed_min, rhs_signop); } } Since we can have PROMOTE_MODE changing the sign and LHS mode and RHS mode precision can be different, the check should be the third one. Does that make sense or am I still missing it? Thanks again for your time, Kugan
On Tue, Aug 05, 2014 at 04:17:41PM +0200, Richard Biener wrote: > what's the semantic of setting SRP_SIGNED_AND_UNSIGNED > on the subreg? That is, for the created (subreg:lhs_mode > (reg:<PROMOTE_MODE of ssa> N))? SRP_SIGNED_AND_UNSIGNED on a subreg should mean that the subreg is both zero and sign extended, which means that the topmost bit of the narrower mode is known to be zero, and all bits above it in the wider mode are known to be zero too. SRP_SIGNED means that the topmost bit of the narrower mode is either 0 or 1 and depending on that the above wider mode bits are either all 0 or all 1. SRP_UNSIGNED means that regardless of the topmost bit value, all above wider mode bits are 0. Jakub
On Tue, Aug 5, 2014 at 4:21 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Aug 05, 2014 at 04:17:41PM +0200, Richard Biener wrote: >> what's the semantic of setting SRP_SIGNED_AND_UNSIGNED >> on the subreg? That is, for the created (subreg:lhs_mode >> (reg:<PROMOTE_MODE of ssa> N))? > > SRP_SIGNED_AND_UNSIGNED on a subreg should mean that > the subreg is both zero and sign extended, which means > that the topmost bit of the narrower mode is known to be zero, > and all bits above it in the wider mode are known to be zero too. > SRP_SIGNED means that the topmost bit of the narrower mode is > either 0 or 1 and depending on that the above wider mode bits > are either all 0 or all 1. > SRP_UNSIGNED means that regardless of the topmost bit value, > all above wider mode bits are 0. Ok, then from the context of the patch we already know that either SRP_UNSIGNED or SRP_SIGNED is true which means that the value is sign- or zero-extended. I suppose inside promoted_for_type_p TYPE_MODE (TREE_TYPE (ssa)) == lhs_mode, I'm not sure why you pass !unsignedp as lhs_uns. Now, from 'ssa' alone we can't tell anything about a larger mode registers value if that is either zero- or sign-extended. But we know that those bits are properly zero-extended if unsignedp and properly sign-extended if !unsignedp? So what the predicate tries to prove is that sign- and zero-extending results in the same larger-mode value. This is true if the MSB of the smaller mode is not set. Let's assume that smaller mode is that of 'ssa' then the test is just return (!tree_int_cst_sign_bit (min) && !tree_int_cst_sign_bit (max)); no? Thanks, Richard. > Jakub
On 06/08/14 22:09, Richard Biener wrote: > On Tue, Aug 5, 2014 at 4:21 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Tue, Aug 05, 2014 at 04:17:41PM +0200, Richard Biener wrote: >>> what's the semantic of setting SRP_SIGNED_AND_UNSIGNED >>> on the subreg? That is, for the created (subreg:lhs_mode >>> (reg:<PROMOTE_MODE of ssa> N))? >> >> SRP_SIGNED_AND_UNSIGNED on a subreg should mean that >> the subreg is both zero and sign extended, which means >> that the topmost bit of the narrower mode is known to be zero, >> and all bits above it in the wider mode are known to be zero too. >> SRP_SIGNED means that the topmost bit of the narrower mode is >> either 0 or 1 and depending on that the above wider mode bits >> are either all 0 or all 1. >> SRP_UNSIGNED means that regardless of the topmost bit value, >> all above wider mode bits are 0. > > Ok, then from the context of the patch we already know that > either SRP_UNSIGNED or SRP_SIGNED is true which means > that the value is sign- or zero-extended. > > I suppose inside promoted_for_type_p > TYPE_MODE (TREE_TYPE (ssa)) == lhs_mode, I'm not sure > why you pass !unsignedp as lhs_uns. In expand_expr_real_1, it is already known that it is promoted for unsigned_p and we are setting SUBREG_PROMOTED_SET (temp, unsignedp). If we can prove that it is also promoted for !unsignedp, we can set SUBREG_PROMOTED_SET (temp, SRP_SIGNED_AND_UNSIGNED). promoted_for_type_p should prove this based on the value range info. > > Now, from 'ssa' alone we can't tell anything about a larger mode > registers value if that is either zero- or sign-extended. But we > know that those bits are properly zero-extended if unsignedp > and properly sign-extended if !unsignedp? > > So what the predicate tries to prove is that sign- and zero-extending > results in the same larger-mode value. This is true if the > MSB of the smaller mode is not set. > > Let's assume that smaller mode is that of 'ssa' then the test > is just > > return (!tree_int_cst_sign_bit (min) && !tree_int_cst_sign_bit (max)); > > no? hmm, is this because we will never have a call to promoted_for_type_p with same sign (ignoring PROMOTE_MODE) for 'ssa' and the larger mode. The case with larger mode signed and 'ssa' unsigned will not work. Therefore larger mode unsigned and 'ssa' signed will be the only case that we should consider. However, with PROMOTE_MODE, isnt that we will miss some cases with this. Thanks, Kugan
On Wed, Aug 6, 2014 at 3:21 PM, Kugan <kugan.vivekanandarajah@linaro.org> wrote: > On 06/08/14 22:09, Richard Biener wrote: >> On Tue, Aug 5, 2014 at 4:21 PM, Jakub Jelinek <jakub@redhat.com> wrote: >>> On Tue, Aug 05, 2014 at 04:17:41PM +0200, Richard Biener wrote: >>>> what's the semantic of setting SRP_SIGNED_AND_UNSIGNED >>>> on the subreg? That is, for the created (subreg:lhs_mode >>>> (reg:<PROMOTE_MODE of ssa> N))? >>> >>> SRP_SIGNED_AND_UNSIGNED on a subreg should mean that >>> the subreg is both zero and sign extended, which means >>> that the topmost bit of the narrower mode is known to be zero, >>> and all bits above it in the wider mode are known to be zero too. >>> SRP_SIGNED means that the topmost bit of the narrower mode is >>> either 0 or 1 and depending on that the above wider mode bits >>> are either all 0 or all 1. >>> SRP_UNSIGNED means that regardless of the topmost bit value, >>> all above wider mode bits are 0. >> >> Ok, then from the context of the patch we already know that >> either SRP_UNSIGNED or SRP_SIGNED is true which means >> that the value is sign- or zero-extended. >> >> I suppose inside promoted_for_type_p >> TYPE_MODE (TREE_TYPE (ssa)) == lhs_mode, I'm not sure >> why you pass !unsignedp as lhs_uns. > > In expand_expr_real_1, it is already known that it is promoted for > unsigned_p and we are setting SUBREG_PROMOTED_SET (temp, unsignedp). > > If we can prove that it is also promoted for !unsignedp, we can set > SUBREG_PROMOTED_SET (temp, SRP_SIGNED_AND_UNSIGNED). > > promoted_for_type_p should prove this based on the value range info. > >> >> Now, from 'ssa' alone we can't tell anything about a larger mode >> registers value if that is either zero- or sign-extended. But we >> know that those bits are properly zero-extended if unsignedp >> and properly sign-extended if !unsignedp? >> >> So what the predicate tries to prove is that sign- and zero-extending >> results in the same larger-mode value. This is true if the >> MSB of the smaller mode is not set. >> >> Let's assume that smaller mode is that of 'ssa' then the test >> is just >> >> return (!tree_int_cst_sign_bit (min) && !tree_int_cst_sign_bit (max)); >> >> no? > > hmm, is this because we will never have a call to promoted_for_type_p > with same sign (ignoring PROMOTE_MODE) for 'ssa' and the larger mode. > The case with larger mode signed and 'ssa' unsigned will not work. > Therefore larger mode unsigned and 'ssa' signed will be the only case > that we should consider. > > However, with PROMOTE_MODE, isnt that we will miss some cases with this. No, PROMOTE_MODE will still either sign- or zero-extend. If either results in zeros in the upper bits then PROMOTE_MODE doesn't matter. Richard. > Thanks, > Kugan > >
diff --git a/gcc/calls.c b/gcc/calls.c index a3e6faa..eac512f 100644 --- a/gcc/calls.c +++ b/gcc/calls.c @@ -1484,7 +1484,10 @@ precompute_arguments (int num_actuals, struct arg_data *args) args[i].initial_value = gen_lowpart_SUBREG (mode, args[i].value); SUBREG_PROMOTED_VAR_P (args[i].initial_value) = 1; - SUBREG_PROMOTED_SET (args[i].initial_value, args[i].unsignedp); + if (is_promoted_for_type (args[i].tree_value, mode, !args[i].unsignedp)) + SUBREG_PROMOTED_SET (args[i].initial_value, SRP_SIGNED_AND_UNSIGNED); + else + SUBREG_PROMOTED_SET (args[i].initial_value, args[i].unsignedp); } } } diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index f98c322..b14626c 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -3309,7 +3309,13 @@ expand_gimple_stmt_1 (gimple stmt) GET_MODE (target), temp, unsignedp); } - convert_move (SUBREG_REG (target), temp, unsignedp); + if ((SUBREG_PROMOTED_GET (target) == SRP_SIGNED_AND_UNSIGNED) + && (GET_CODE (temp) == SUBREG) + && (GET_MODE (target) == GET_MODE (temp)) + && (GET_MODE (SUBREG_REG (target)) == GET_MODE (SUBREG_REG (temp)))) + emit_move_insn (SUBREG_REG (target), SUBREG_REG (temp)); + else + convert_move (SUBREG_REG (target), temp, unsignedp); } else if (nontemporal && emit_storent_insn (target, temp)) ; diff --git a/gcc/expr.c b/gcc/expr.c index 7356e76..d25f506 100644 --- a/gcc/expr.c +++ b/gcc/expr.c @@ -68,6 +68,7 @@ along with GCC; see the file COPYING3. If not see #include "tree-ssa-address.h" #include "cfgexpand.h" #include "builtins.h" +#include "tree-ssa.h" #ifndef STACK_PUSH_CODE #ifdef STACK_GROWS_DOWNWARD @@ -9224,6 +9225,65 @@ expand_expr_real_2 (sepops ops, rtx target, enum machine_mode tmode, } #undef REDUCE_BIT_FIELD +/* Return TRUE if value in SSA is already zero/sign extended for lhs type + (type here is the combination of LHS_MODE and LHS_UNS) using value range + information stored. Return FALSE otherwise. */ +bool +is_promoted_for_type (tree ssa, enum machine_mode lhs_mode, bool lhs_uns) +{ + wide_int type_min, type_max; + wide_int min, max, limit; + unsigned int prec; + tree lhs_type; + bool rhs_uns; + enum machine_mode mode; + + if (ssa == NULL_TREE + || TREE_CODE (ssa) != SSA_NAME + || !INTEGRAL_TYPE_P (TREE_TYPE (ssa))) + return false; + + /* Return FALSE if value_range is not recorded for SSA. */ + if (get_range_info (ssa, &min, &max) != VR_RANGE) + return false; + + /* In some architectures, modes are promoted and sign changed with + target defined PROMOTE_MODE macro. If PROMOTE_MODE tells you to + promote _not_ according to ssa's sign then honour that. */ + mode = TYPE_MODE (TREE_TYPE (ssa)); + rhs_uns = TYPE_UNSIGNED (TREE_TYPE (ssa)); + PROMOTE_MODE (mode, rhs_uns, TREE_TYPE (ssa)); + + lhs_type = lang_hooks.types.type_for_mode (lhs_mode, lhs_uns); + prec = min.get_precision (); + + /* Signed maximum value. */ + limit = wide_int::from (TYPE_MAX_VALUE (TREE_TYPE (ssa)), prec, SIGNED); + + /* Signedness of LHS and RHS differs but values in range. */ + if ((rhs_uns != lhs_uns) + && ((!lhs_uns && !wi::neg_p (min, TYPE_SIGN (lhs_type))) + || (lhs_uns && (wi::cmp (max, limit, TYPE_SIGN (TREE_TYPE (ssa))) == -1)))) + lhs_uns = !lhs_uns; + + /* Signedness of LHS and RHS should match. */ + if (rhs_uns != lhs_uns) + return false; + + 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))); + + /* Check if values lies in-between the type range. */ + if ((wi::neg_p (max, TYPE_SIGN (TREE_TYPE (ssa))) + || (wi::cmp (max, type_max, TYPE_SIGN (TREE_TYPE (ssa))) != 1)) + && (!wi::neg_p (min, TYPE_SIGN (TREE_TYPE (ssa))) + || (wi::cmp (type_min, min, TYPE_SIGN (TREE_TYPE (ssa))) != 1))) + return true; + + return false; +} /* Return TRUE if expression STMT is suitable for replacement. Never consider memory loads as replaceable, because those don't ever lead @@ -9527,7 +9587,10 @@ expand_expr_real_1 (tree exp, rtx target, enum machine_mode tmode, temp = gen_lowpart_SUBREG (mode, decl_rtl); SUBREG_PROMOTED_VAR_P (temp) = 1; - SUBREG_PROMOTED_SET (temp, unsignedp); + if (is_promoted_for_type (ssa_name, mode, !unsignedp)) + SUBREG_PROMOTED_SET (temp, SRP_SIGNED_AND_UNSIGNED); + else + SUBREG_PROMOTED_SET (temp, unsignedp); return temp; } diff --git a/gcc/expr.h b/gcc/expr.h index 6a1d3ab..e99d000 100644 --- a/gcc/expr.h +++ b/gcc/expr.h @@ -440,6 +440,7 @@ extern rtx expand_expr_real_1 (tree, rtx, enum machine_mode, enum expand_modifier, rtx *, bool); extern rtx expand_expr_real_2 (sepops, rtx, enum machine_mode, enum expand_modifier); +extern bool is_promoted_for_type (tree, enum machine_mode, bool); /* Generate code for computing expression EXP. An rtx for the computed value is returned. The value is never null. diff --git a/gcc/testsuite/gcc.dg/zero_sign_ext_test.c b/gcc/testsuite/gcc.dg/zero_sign_ext_test.c index e69de29..6a52678 100644 --- a/gcc/testsuite/gcc.dg/zero_sign_ext_test.c +++ b/gcc/testsuite/gcc.dg/zero_sign_ext_test.c @@ -0,0 +1,136 @@ +extern void abort (void); + +/* { dg-options "-O2" } */ +/* { dg-do run } */ + +#define TYPE_MAX(type, sign) \ + ((!sign) ? ((1 << (sizeof (type) * 8 - 1)) - 1) : \ + ((1 << (sizeof (type) * 8)) - 1)) +#define TYPE_MIN(type, sign) \ + ((!sign) ? -(1 << (sizeof (type) * 8 - 1)) : 0) + +#define TEST_FN(NAME, ARG_TYPE, RET_TYPE, CAST_TYPE, VAL, VR_MIN, VR_MAX)\ + __attribute__((noinline, noclone)) RET_TYPE \ + NAME (ARG_TYPE arg){ \ + RET_TYPE ret = VAL; \ + if (arg + 1 < VR_MIN || arg + 1 > VR_MAX) return ret; \ + /* Value Range of arg at this point will be [VR_min, VR_max]. */\ + arg = arg + VAL; \ + ret = (CAST_TYPE)arg; \ + return arg; \ + } + +/* Signed to signed conversion with value in-range. */ +TEST_FN (foo1, short, short, char, 1, TYPE_MIN (char, 0), TYPE_MAX (char, 0)); +TEST_FN (foo2, short, short, char, 1, TYPE_MIN (char, 0) + 1,\ + TYPE_MAX (char, 0) - 1); + +/* Signed to signed conversion with value not in-range. */ +TEST_FN (foo3, short, short, char, -1, TYPE_MIN (short, 0) + 1, 100); +TEST_FN (foo4, short, short, char, 1, 12, TYPE_MAX (short, 0) + 1); + +/* Unsigned to unsigned conversion with value in-range. */ +TEST_FN (foo5, unsigned short, unsigned short, unsigned char, 1,\ + TYPE_MIN (char, 1) + 1, TYPE_MAX (char, 1) - 1); +TEST_FN (foo6, unsigned short, unsigned short, unsigned char, 1,\ + TYPE_MIN (char, 1), TYPE_MAX (char, 1)); + +/* Unsigned to unsigned conversion with value not in-range. */ +TEST_FN (foo7, unsigned short, unsigned short, unsigned char, 1,\ + TYPE_MIN (short, 1) + 1, TYPE_MAX (short, 1) - 1); +TEST_FN (foo8, unsigned short, unsigned short, unsigned char, 1,\ + TYPE_MIN (short, 1), TYPE_MAX (short, 1)); + +/* Signed to unsigned conversion with value range positive. */ +TEST_FN (foo9, short, short, unsigned char, -1, 1,\ + TYPE_MAX (char, 1) - 1); +TEST_FN (foo10, short, short, unsigned char, 1, 0,\ + TYPE_MAX (char, 1)); + +/* Signed to unsigned conversion with value range negative. */ +TEST_FN (foo11, short, short, unsigned char, 1,\ + TYPE_MIN (char, 0) + 1, TYPE_MAX (char, 0) - 1); +TEST_FN (foo12, short, short, unsigned char, 1,\ + TYPE_MIN (char, 0), TYPE_MAX (char, 0)); + +/* Unsigned to Signed conversion with value range in signed equiv range. */ +TEST_FN (foo13, unsigned short, unsigned short, char, 1,\ + TYPE_MIN (char, 1) + 1, TYPE_MAX (char, 0) - 1); +TEST_FN (foo14, unsigned short, unsigned short, char, 1,\ + TYPE_MIN (char, 1), TYPE_MAX (char, 0)); + +/* Unsigned to Signed conversion with value range not-in signed range. */ +TEST_FN (foo15, unsigned short, unsigned short, char, 1,\ + TYPE_MIN (char, 1) + 1, TYPE_MAX (char, 1) - 1); +TEST_FN (foo16, unsigned short, unsigned short, char, 1,\ + TYPE_MIN (char, 1), TYPE_MAX (char, 1)); + +int main () +{ + /* Signed to signed conversion with value in-range. */ + /* arg + 1. */ + if (foo1 (-32) != -31) + abort (); + /* arg + 1. */ + if (foo2 (32) != 33) + abort (); + + /* Signed to signed conversion with value not in-range. */ + /* arg - 1. */ + if (foo3 (-512) != -513) + abort (); + /* arg + 1. */ + if (foo4 (512) != 513) + abort (); + + /* Unsigned to unsigned conversion with value in-range. */ + /* arg + 1. */ + if (foo5 (64) != 65) + abort (); + /* arg + 1. */ + if (foo6 (64) != 65) + abort (); + + /* Unsigned to unsigned conversion with value not in-range. */ + /* arg + 1. */ + if (foo7 (512) != 513) + abort (); + /* arg + 1. */ + if (foo8 (512) != 513) + abort (); + + /* Signed to unsigned conversion with value range positive. */ + /* arg - 1. */ + if (foo9 (2) != 1) + abort (); + /* arg + 1. */ + if (foo10 (2) != 3) + abort (); + + /* Signed to unsigned conversion with value range negative. */ + /* arg + 1. */ + if (foo11 (-125) != -124) + abort (); + /* arg + 1. */ + if (foo12 (-125) != -124) + abort (); + + /* Unsigned to Signed conversion with value range in signed equiv range. */ + /* arg + 1. */ + if (foo13 (125) != 126) + abort (); + /* arg + 1. */ + if (foo14 (125) != 126) + abort (); + + /* Unsigned to Signed conversion with value range not-in signed range. */ + /* arg + 1. */ + if (foo15 (250) != 251) + abort (); + /* arg + 1. */ + if (foo16 (250) != 251) + abort (); + + return 0; +} +