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?
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.
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;
+}
+
