Re: [RFA] [middle-end/54041] Convert modes as needed from expand_expr
On Mon, Feb 10, 2014 at 5:35 PM, Jeff Law wrote:
> On 02/07/14 02:17, Richard Biener wrote:
>>>
>>> +2014-02-05 Jeff Law
>>> +
>>> + PR middle-end/54041
>>> + * expr.c (expand_expr_addr_1): Handle expand_expr returning an
>>> + object with an undesirable mode.
>>> +
>>> 2014-02-05 Bill Schmidt
>>>
>>> * config/rs6000/rs6000.c (altivec_expand_vec_perm_const): Change
>>> diff --git a/gcc/expr.c b/gcc/expr.c
>>> index 878a51b..9609c45 100644
>>> --- a/gcc/expr.c
>>> +++ b/gcc/expr.c
>>> @@ -7708,6 +7708,11 @@ expand_expr_addr_expr_1 (tree exp, rtx target,
>>> enum
>>> machine_mode tmode,
>>> modifier == EXPAND_INITIALIZER
>>>? EXPAND_INITIALIZER : EXPAND_NORMAL);
>>>
>>> + /* expand_expr is allowed to return an object in a mode other
>>> +than TMODE. If it did, we need to convert. */
>>> + if (tmode != GET_MODE (tmp))
>>> + tmp = convert_modes (tmode, GET_MODE (tmp),
>>> +tmp, TYPE_UNSIGNED (TREE_TYPE (offset)));
>>
>>
>> What about CONSTANT_P tmp? Don't you need to use
>> TYPE_MODE (TREE_TYPE (offset)) in that case?
>
>
> As I mentioned last week, we want to pass VOIDmode objects (constants) down
> to convert_memory_address_addr_space unchange. c_m_a_a_s will handle those
> correctly.
>
> This patch fixes that oversight and the function name in the ChangeLog
> entry.
>
> I've verified this version still fixes the original bug report and included
> it in an x86_64-unknown-linux-gnu bootstrap & test for sanity's sake.
>
>
> OK for the trunk?
Ok.
Thanks,
Richard.
> Thanks,
> Jeff
>
>
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 2dbab72..eca3e2f 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,9 @@
> +2014-02-05 Jeff Law
> +
> + PR middle-end/54041
> + * expr.c (expand_expr_addr_expr_1): Handle expand_expr returning an
> + object with an undesirable mode.
> +
> 2014-02-05 Bill Schmidt
>
> * config/rs6000/rs6000.c (altivec_expand_vec_perm_const): Change
> diff --git a/gcc/expr.c b/gcc/expr.c
> index 878a51b..42a451d 100644
> --- a/gcc/expr.c
> +++ b/gcc/expr.c
> @@ -7708,6 +7708,11 @@ expand_expr_addr_expr_1 (tree exp, rtx target, enum
> machine_mode tmode,
> modifier == EXPAND_INITIALIZER
> ? EXPAND_INITIALIZER : EXPAND_NORMAL);
>
> + /* expand_expr is allowed to return an object in a mode other
> +than TMODE. If it did, we need to convert. */
> + if (GET_MODE (tmp) != VOIDmode && tmode != GET_MODE (tmp))
> + tmp = convert_modes (tmode, GET_MODE (tmp),
> +tmp, TYPE_UNSIGNED (TREE_TYPE (offset)));
>result = convert_memory_address_addr_space (tmode, result, as);
>tmp = convert_memory_address_addr_space (tmode, tmp, as);
>
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index c81a00d..283912d 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-02-05 Jeff Law
> +
> + PR middle-end/54041
> + * gcc.target/m68k/pr54041.c: New test.
> +
> 2014-02-05 Bill Schmidt
>
> * gcc.dg/vmx/sum2s.c: New.
> diff --git a/gcc/testsuite/gcc.target/m68k/pr54041.c
> b/gcc/testsuite/gcc.target/m68k/pr54041.c
> new file mode 100644
> index 000..645cb6d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/m68k/pr54041.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -mshort" } */
> +
> +extern int r[];
> +
> +int *fn(int i)
> +{
> + return &r[i];
> +}
> +
>
Re: [RFA] [middle-end/54041] Convert modes as needed from expand_expr
On 02/07/14 02:17, Richard Biener wrote:
+2014-02-05 Jeff Law
+
+ PR middle-end/54041
+ * expr.c (expand_expr_addr_1): Handle expand_expr returning an
+ object with an undesirable mode.
+
2014-02-05 Bill Schmidt
* config/rs6000/rs6000.c (altivec_expand_vec_perm_const): Change
diff --git a/gcc/expr.c b/gcc/expr.c
index 878a51b..9609c45 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -7708,6 +7708,11 @@ expand_expr_addr_expr_1 (tree exp, rtx target, enum
machine_mode tmode,
modifier == EXPAND_INITIALIZER
? EXPAND_INITIALIZER : EXPAND_NORMAL);
+ /* expand_expr is allowed to return an object in a mode other
+than TMODE. If it did, we need to convert. */
+ if (tmode != GET_MODE (tmp))
+ tmp = convert_modes (tmode, GET_MODE (tmp),
+tmp, TYPE_UNSIGNED (TREE_TYPE (offset)));
What about CONSTANT_P tmp? Don't you need to use
TYPE_MODE (TREE_TYPE (offset)) in that case?
As I mentioned last week, we want to pass VOIDmode objects (constants)
down to convert_memory_address_addr_space unchange. c_m_a_a_s will
handle those correctly.
This patch fixes that oversight and the function name in the ChangeLog
entry.
I've verified this version still fixes the original bug report and
included it in an x86_64-unknown-linux-gnu bootstrap & test for sanity's
sake.
OK for the trunk?
Thanks,
Jeff
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 2dbab72..eca3e2f 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,9 @@
+2014-02-05 Jeff Law
+
+ PR middle-end/54041
+ * expr.c (expand_expr_addr_expr_1): Handle expand_expr returning an
+ object with an undesirable mode.
+
2014-02-05 Bill Schmidt
* config/rs6000/rs6000.c (altivec_expand_vec_perm_const): Change
diff --git a/gcc/expr.c b/gcc/expr.c
index 878a51b..42a451d 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -7708,6 +7708,11 @@ expand_expr_addr_expr_1 (tree exp, rtx target, enum
machine_mode tmode,
modifier == EXPAND_INITIALIZER
? EXPAND_INITIALIZER : EXPAND_NORMAL);
+ /* expand_expr is allowed to return an object in a mode other
+than TMODE. If it did, we need to convert. */
+ if (GET_MODE (tmp) != VOIDmode && tmode != GET_MODE (tmp))
+ tmp = convert_modes (tmode, GET_MODE (tmp),
+tmp, TYPE_UNSIGNED (TREE_TYPE (offset)));
result = convert_memory_address_addr_space (tmode, result, as);
tmp = convert_memory_address_addr_space (tmode, tmp, as);
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index c81a00d..283912d 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-02-05 Jeff Law
+
+ PR middle-end/54041
+ * gcc.target/m68k/pr54041.c: New test.
+
2014-02-05 Bill Schmidt
* gcc.dg/vmx/sum2s.c: New.
diff --git a/gcc/testsuite/gcc.target/m68k/pr54041.c
b/gcc/testsuite/gcc.target/m68k/pr54041.c
new file mode 100644
index 000..645cb6d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/m68k/pr54041.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-O -mshort" } */
+
+extern int r[];
+
+int *fn(int i)
+{
+ return &r[i];
+}
+
Re: [RFA] [middle-end/54041] Convert modes as needed from expand_expr
On 02/07/14 02:17, Richard Biener wrote: diff --git a/gcc/expr.c b/gcc/expr.c index 878a51b..9609c45 100644 --- a/gcc/expr.c +++ b/gcc/expr.c @@ -7708,6 +7708,11 @@ expand_expr_addr_expr_1 (tree exp, rtx target, enum machine_mode tmode, modifier == EXPAND_INITIALIZER ? EXPAND_INITIALIZER : EXPAND_NORMAL); + /* expand_expr is allowed to return an object in a mode other +than TMODE. If it did, we need to convert. */ + if (tmode != GET_MODE (tmp)) + tmp = convert_modes (tmode, GET_MODE (tmp), +tmp, TYPE_UNSIGNED (TREE_TYPE (offset))); What about CONSTANT_P tmp? Don't you need to use TYPE_MODE (TREE_TYPE (offset)) in that case? Good question. Given the behaviour of c_m_a_a_s, we should do nothing if GET_MODE (tmp) == VOIDmode. I'll update the patch and resubmit. jeff
Re: [RFA] [middle-end/54041] Convert modes as needed from expand_expr
On Thu, Feb 6, 2014 at 8:33 PM, Jeff Law wrote:
>
> expand_expr has, for as long as I can remember, had the ability to ignore
> the desired mode provided by its callers and instead returning something in
> a completely different mode. It's always been the caller's responsibility
> to deal with that.
>
> For the testcase in 54041, we call expand_expr with a desired mode of
> SImode, but it actually returns a HImode object. This causes the assertion
> in convert_memory_address_addr_space to trip because the passed mode must be
> the same as the mode of the memory address.
>
> The fix is simple. If expand_expr returns something in the wrong mode,
> convert it to the desired mode.
>
> I've reviewed the resulting code for the m68k target and it looks correct to
> me. I've also bootstrapped and regression tested on
> x86_64-unknown-linux-gnu, though the new code most certainly does not
> trigger there.
>
> I guess if someone really wanted to be thorough, they'd test on a target
> where pointers and integers are different sizes.
>
> OK for the trunk?
>
> Thanks,
> Jeff
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 2dbab72..4c7da83 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,9 @@
> +2014-02-05 Jeff Law
> +
> + PR middle-end/54041
> + * expr.c (expand_expr_addr_1): Handle expand_expr returning an
> + object with an undesirable mode.
> +
> 2014-02-05 Bill Schmidt
>
> * config/rs6000/rs6000.c (altivec_expand_vec_perm_const): Change
> diff --git a/gcc/expr.c b/gcc/expr.c
> index 878a51b..9609c45 100644
> --- a/gcc/expr.c
> +++ b/gcc/expr.c
> @@ -7708,6 +7708,11 @@ expand_expr_addr_expr_1 (tree exp, rtx target, enum
> machine_mode tmode,
> modifier == EXPAND_INITIALIZER
> ? EXPAND_INITIALIZER : EXPAND_NORMAL);
>
> + /* expand_expr is allowed to return an object in a mode other
> +than TMODE. If it did, we need to convert. */
> + if (tmode != GET_MODE (tmp))
> + tmp = convert_modes (tmode, GET_MODE (tmp),
> +tmp, TYPE_UNSIGNED (TREE_TYPE (offset)));
What about CONSTANT_P tmp? Don't you need to use
TYPE_MODE (TREE_TYPE (offset)) in that case?
Richard.
>result = convert_memory_address_addr_space (tmode, result, as);
>tmp = convert_memory_address_addr_space (tmode, tmp, as);
>
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index c81a00d..283912d 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-02-05 Jeff Law
> +
> + PR middle-end/54041
> + * gcc.target/m68k/pr54041.c: New test.
> +
> 2014-02-05 Bill Schmidt
>
> * gcc.dg/vmx/sum2s.c: New.
> diff --git a/gcc/testsuite/gcc.target/m68k/pr54041.c
> b/gcc/testsuite/gcc.target/m68k/pr54041.c
> new file mode 100644
> index 000..645cb6d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/m68k/pr54041.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -mshort" } */
> +
> +extern int r[];
> +
> +int *fn(int i)
> +{
> + return &r[i];
> +}
> +
>
[RFA] [middle-end/54041] Convert modes as needed from expand_expr
expand_expr has, for as long as I can remember, had the ability to
ignore the desired mode provided by its callers and instead returning
something in a completely different mode. It's always been the caller's
responsibility to deal with that.
For the testcase in 54041, we call expand_expr with a desired mode of
SImode, but it actually returns a HImode object. This causes the
assertion in convert_memory_address_addr_space to trip because the
passed mode must be the same as the mode of the memory address.
The fix is simple. If expand_expr returns something in the wrong mode,
convert it to the desired mode.
I've reviewed the resulting code for the m68k target and it looks
correct to me. I've also bootstrapped and regression tested on
x86_64-unknown-linux-gnu, though the new code most certainly does not
trigger there.
I guess if someone really wanted to be thorough, they'd test on a target
where pointers and integers are different sizes.
OK for the trunk?
Thanks,
Jeff
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 2dbab72..4c7da83 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,9 @@
+2014-02-05 Jeff Law
+
+ PR middle-end/54041
+ * expr.c (expand_expr_addr_1): Handle expand_expr returning an
+ object with an undesirable mode.
+
2014-02-05 Bill Schmidt
* config/rs6000/rs6000.c (altivec_expand_vec_perm_const): Change
diff --git a/gcc/expr.c b/gcc/expr.c
index 878a51b..9609c45 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -7708,6 +7708,11 @@ expand_expr_addr_expr_1 (tree exp, rtx target, enum
machine_mode tmode,
modifier == EXPAND_INITIALIZER
? EXPAND_INITIALIZER : EXPAND_NORMAL);
+ /* expand_expr is allowed to return an object in a mode other
+than TMODE. If it did, we need to convert. */
+ if (tmode != GET_MODE (tmp))
+ tmp = convert_modes (tmode, GET_MODE (tmp),
+tmp, TYPE_UNSIGNED (TREE_TYPE (offset)));
result = convert_memory_address_addr_space (tmode, result, as);
tmp = convert_memory_address_addr_space (tmode, tmp, as);
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index c81a00d..283912d 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-02-05 Jeff Law
+
+ PR middle-end/54041
+ * gcc.target/m68k/pr54041.c: New test.
+
2014-02-05 Bill Schmidt
* gcc.dg/vmx/sum2s.c: New.
diff --git a/gcc/testsuite/gcc.target/m68k/pr54041.c
b/gcc/testsuite/gcc.target/m68k/pr54041.c
new file mode 100644
index 000..645cb6d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/m68k/pr54041.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-O -mshort" } */
+
+extern int r[];
+
+int *fn(int i)
+{
+ return &r[i];
+}
+
