On 18/01/17 21:07, Jeff Law wrote:
> On 01/18/2017 11:08 AM, Richard Earnshaw (lists) wrote:
>> PR 79121 is a silent wrong code regression where, when generating a
>> shift from an extended value moving from one to two machine registers,
>> the type of the right shift is for the most significant word should be
>> determined by the signedness of the inner type, not the signedness of
>> the result type.
>>
>> gcc:
>> PR rtl-optimization/79121
>> * expr.c (expand_expr_real_2, case LSHIFT_EXPR): Look at the
>> signedness
>> of the inner type when shifting an extended value.
>>
>> testsuite:
>> * gcc.c-torture/execute/pr79121.c: New test.
>>
>> Bootstrapped on x86_64 and cross-tested on ARM.
> I had to refamiliarize myself with this code and nearly got the analysis
> wrong (again).
>
> Due to the copying of the low word into the high word we have to select
> the type of shift based on the type of the object that was the source of
> the NOP conversion. The code currently makes that determination based
> on the signedness of the shift, which is wrong.
>
>
> OK for the trunk.
>
> jeff
>
>
Thanks, Jeff. I made some minor tweaks to the comments (adding a bit
more about signed vs unsigned) and committed the following.
What about gcc-6?
R.
diff --git a/gcc/expr.c b/gcc/expr.c
index 4c54faf..2d8868e 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -9056,9 +9056,9 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode
tmode,
/* Left shift optimization when shifting across word_size boundary.
- If mode == GET_MODE_WIDER_MODE (word_mode), then normally there isn't
- native instruction to support this wide mode left shift. Given below
- scenario:
+ If mode == GET_MODE_WIDER_MODE (word_mode), then normally
+ there isn't native instruction to support this wide mode
+ left shift. Given below scenario:
Type A = (Type) B << C
@@ -9067,10 +9067,11 @@ expand_expr_real_2 (sepops ops, rtx target,
machine_mode tmode,
| word_size |
- If the shift amount C caused we shift B to across the word size
- boundary, i.e part of B shifted into high half of destination
- register, and part of B remains in the low half, then GCC will use
- the following left shift expand logic:
+ If the shift amount C caused we shift B to across the word
+ size boundary, i.e part of B shifted into high half of
+ destination register, and part of B remains in the low
+ half, then GCC will use the following left shift expand
+ logic:
1. Initialize dest_low to B.
2. Initialize every bit of dest_high to the sign bit of B.
@@ -9080,20 +9081,30 @@ expand_expr_real_2 (sepops ops, rtx target,
machine_mode tmode,
5. Logic right shift D by (word_size - C).
6. Or the result of 4 and 5 to finalize dest_high.
- While, by checking gimple statements, if operand B is coming from
- signed extension, then we can simplify above expand logic into:
+ While, by checking gimple statements, if operand B is
+ coming from signed extension, then we can simplify above
+ expand logic into:
1. dest_high = src_low >> (word_size - C).
2. dest_low = src_low << C.
- We can use one arithmetic right shift to finish all the purpose of
- steps 2, 4, 5, 6, thus we reduce the steps needed from 6 into 2. */
+ We can use one arithmetic right shift to finish all the
+ purpose of steps 2, 4, 5, 6, thus we reduce the steps
+ needed from 6 into 2.
+
+ The case is similar for zero extension, except that we
+ initialize dest_high to zero rather than copies of the sign
+ bit from B. Furthermore, we need to use a logical right shift
+ in this case.
+
+ The choice of sign-extension versus zero-extension is
+ determined entirely by whether or not B is signed and is
+ independent of the current setting of unsignedp. */
temp = NULL_RTX;
if (code == LSHIFT_EXPR
&& target
&& REG_P (target)
- && ! unsignedp
&& mode == GET_MODE_WIDER_MODE (word_mode)
&& GET_MODE_SIZE (mode) == 2 * GET_MODE_SIZE (word_mode)
&& TREE_CONSTANT (treeop1)
@@ -9114,6 +9125,8 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode
tmode,
rtx_insn *seq, *seq_old;
unsigned int high_off = subreg_highpart_offset (word_mode,
mode);
+ bool extend_unsigned
+ = TYPE_UNSIGNED (TREE_TYPE (gimple_assign_rhs1 (def)));
rtx low = lowpart_subreg (word_mode, op0, mode);
rtx dest_low = lowpart_subreg (word_mode, target, mode);
rtx dest_high = simplify_gen_subreg (word_mode, target,
@@ -9125,7 +9138,8 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode
tmode,
start_sequence ();
/* dest_high = src_low >> (word_size - C). */
temp = expand_variable_shift (RSHIFT_EXPR, word_mode, low,
- rshift, dest_high, unsignedp);
+ rshift, dest_high,
+ extend_unsigned);
if (temp != dest_high)
emit_move_insn (dest_high, temp);
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr79121.c
b/gcc/testsuite/gcc.c-torture/execute/pr79121.c
new file mode 100644
index 0000000..9fca7fb
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr79121.c
@@ -0,0 +1,34 @@
+extern void abort (void);
+
+__attribute__ ((noinline, noclone)) unsigned long long f1 (int x)
+{
+ return ((unsigned long long) x) << 4;
+}
+
+__attribute__ ((noinline, noclone)) long long f2 (unsigned x)
+{
+ return ((long long) x) << 4;
+}
+
+__attribute__ ((noinline, noclone)) unsigned long long f3 (unsigned x)
+{
+ return ((unsigned long long) x) << 4;
+}
+
+__attribute__ ((noinline, noclone)) long long f4 (int x)
+{
+ return ((long long) x) << 4;
+}
+
+int main ()
+{
+ if (f1 (0xf0000000) != 0xffffffff00000000)
+ abort ();
+ if (f2 (0xf0000000) != 0xf00000000)
+ abort ();
+ if (f3 (0xf0000000) != 0xf00000000)
+ abort ();
+ if (f4 (0xf0000000) != 0xffffffff00000000)
+ abort ();
+ return 0;
+}
