* [PATCH 2/6] cris: Fix for RTL checking
2017-02-21 14:48 [PATCH 0/6] Fallout from RTL checking Segher Boessenkool
@ 2017-02-21 14:48 ` Segher Boessenkool
2017-02-21 15:03 ` Hans-Peter Nilsson
2017-02-21 14:49 ` [PATCH 1/6] c6x: " Segher Boessenkool
` (5 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Segher Boessenkool @ 2017-02-21 14:48 UTC (permalink / raw)
To: gcc-patches; +Cc: Segher Boessenkool, hp
2017-02-21 Segher Boessenkool <segher@kernel.crashing.org>
* config/cris/cris.md: Use correct operand in a define_peephole2.
---
gcc/config/cris/cris.md | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/config/cris/cris.md b/gcc/config/cris/cris.md
index 6ba3772..472aec7 100644
--- a/gcc/config/cris/cris.md
+++ b/gcc/config/cris/cris.md
@@ -5034,7 +5034,7 @@ (define_peephole2 ; andqu (casesi+46)
[(set (match_dup 0) (match_dup 3))
(set (match_dup 0) (and:SI (match_dup 0) (match_dup 4)))]
{
- machine_mode zmode = INTVAL (operands[2]) <= 255 ? QImode : HImode;
+ machine_mode zmode = INTVAL (operands[1]) <= 255 ? QImode : HImode;
rtx op1
= (REG_S_P (operands[2])
? gen_rtx_REG (zmode, REGNO (operands[2]))
--
1.9.3
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 2/6] cris: Fix for RTL checking
2017-02-21 14:48 ` [PATCH 2/6] cris: Fix for " Segher Boessenkool
@ 2017-02-21 15:03 ` Hans-Peter Nilsson
0 siblings, 0 replies; 27+ messages in thread
From: Hans-Peter Nilsson @ 2017-02-21 15:03 UTC (permalink / raw)
To: segher; +Cc: gcc-patches, segher
> From: Segher Boessenkool <segher@kernel.crashing.org>
> Date: Tue, 21 Feb 2017 14:48:14 +0000
> 2017-02-21 Segher Boessenkool <segher@kernel.crashing.org>
>
> * config/cris/cris.md: Use correct operand in a define_peephole2.
Obviously ok, thanks (INTVAL on "const_int_operand"
vs. "nonimmediate_operand").
brgds, H-P
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 1/6] c6x: Fix for RTL checking
2017-02-21 14:48 [PATCH 0/6] Fallout from RTL checking Segher Boessenkool
2017-02-21 14:48 ` [PATCH 2/6] cris: Fix for " Segher Boessenkool
@ 2017-02-21 14:49 ` Segher Boessenkool
2017-02-21 15:09 ` Bernd Schmidt
2017-02-21 14:49 ` [PATCH 4/6] nios2: Fixes " Segher Boessenkool
` (4 subsequent siblings)
6 siblings, 1 reply; 27+ messages in thread
From: Segher Boessenkool @ 2017-02-21 14:49 UTC (permalink / raw)
To: gcc-patches; +Cc: Segher Boessenkool, bschmidt
2017-02-21 Segher Boessenkool <segher@kernel.crashing.org>
* config/c6x/c6x.c (predicate_insn): Do not incorrectly share RTL.
---
gcc/config/c6x/c6x.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/gcc/config/c6x/c6x.c b/gcc/config/c6x/c6x.c
index 84bfdfa..42b773b 100644
--- a/gcc/config/c6x/c6x.c
+++ b/gcc/config/c6x/c6x.c
@@ -3800,6 +3800,7 @@ predicate_insn (rtx_insn *insn, rtx cond, bool doit)
{
if (doit)
{
+ cond = copy_rtx (cond);
rtx newpat = gen_rtx_COND_EXEC (VOIDmode, cond, PATTERN (insn));
PATTERN (insn) = newpat;
INSN_CODE (insn) = -1;
--
1.9.3
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH 4/6] nios2: Fixes for RTL checking
2017-02-21 14:48 [PATCH 0/6] Fallout from RTL checking Segher Boessenkool
2017-02-21 14:48 ` [PATCH 2/6] cris: Fix for " Segher Boessenkool
2017-02-21 14:49 ` [PATCH 1/6] c6x: " Segher Boessenkool
@ 2017-02-21 14:49 ` Segher Boessenkool
2017-02-21 19:33 ` Jeff Law
2017-02-21 19:53 ` Sandra Loosemore
2017-02-21 14:49 ` [PATCH 3/6] microblaze: " Segher Boessenkool
` (3 subsequent siblings)
6 siblings, 2 replies; 27+ messages in thread
From: Segher Boessenkool @ 2017-02-21 14:49 UTC (permalink / raw)
To: gcc-patches; +Cc: Segher Boessenkool, sandra, bschmidt
You cannot call REGNO on something that isn't a REG, and you cannot
call INTVAL on something that isn't a CONST_INT.
The way I fixed nios2_alternate_compare_const is admittedly a bit lame.
2017-02-21 Segher Boessenkool <segher@kernel.crashing.org>
* config/nios2/nios2.c (nios2_simple_const_p): Returns false if the
argument isn't a CONST_INT.
(nios2_alternate_compare_const): Set *alt_code and *alt_op to code
and op if op is not a CONST_INT.
(nios2_valid_compare_const_p): Return false if the argument isn't
a CONST_INT.
(ldstwm_operation_p): Return false if first_base is not a REG or
if first_offset is not a CONST_INT.
---
gcc/config/nios2/nios2.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/gcc/config/nios2/nios2.c b/gcc/config/nios2/nios2.c
index e1b0372..75eeee3 100644
--- a/gcc/config/nios2/nios2.c
+++ b/gcc/config/nios2/nios2.c
@@ -1416,6 +1416,8 @@ nios2_option_override (void)
static bool
nios2_simple_const_p (const_rtx cst)
{
+ if (!CONST_INT_P (cst))
+ return false;
HOST_WIDE_INT val = INTVAL (cst);
return SMALL_INT (val) || SMALL_INT_UNSIGNED (val) || UPPER16_INT (val);
}
@@ -1753,6 +1755,13 @@ nios2_alternate_compare_const (enum rtx_code code, rtx op,
enum rtx_code *alt_code, rtx *alt_op,
machine_mode mode)
{
+ if (!CONST_INT_P (op))
+ {
+ *alt_code = code;
+ *alt_op = op;
+ return;
+ }
+
HOST_WIDE_INT opval = INTVAL (op);
enum rtx_code scode = signed_condition (code);
bool dec_p = (scode == LT || scode == GE);
@@ -1788,6 +1797,9 @@ nios2_alternate_compare_const (enum rtx_code code, rtx op,
static bool
nios2_valid_compare_const_p (enum rtx_code code, rtx op)
{
+ if (!CONST_INT_P (op))
+ return false;
+
switch (code)
{
case EQ: case NE: case GE: case LT:
@@ -4558,6 +4570,8 @@ ldstwm_operation_p (rtx op, bool load_p)
if (!split_mem_address (XEXP (mem, 0),
&first_base, &first_offset))
return false;
+ if (!REG_P (first_base) || !CONST_INT_P (first_offset))
+ return false;
base_reg = first_base;
inc_p = INTVAL (first_offset) >= 0;
}
--
1.9.3
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 4/6] nios2: Fixes for RTL checking
2017-02-21 14:49 ` [PATCH 4/6] nios2: Fixes " Segher Boessenkool
@ 2017-02-21 19:33 ` Jeff Law
2017-02-21 19:53 ` Sandra Loosemore
1 sibling, 0 replies; 27+ messages in thread
From: Jeff Law @ 2017-02-21 19:33 UTC (permalink / raw)
To: Segher Boessenkool, gcc-patches; +Cc: sandra, bschmidt
On 02/21/2017 07:48 AM, Segher Boessenkool wrote:
> You cannot call REGNO on something that isn't a REG, and you cannot
> call INTVAL on something that isn't a CONST_INT.
>
> The way I fixed nios2_alternate_compare_const is admittedly a bit lame.
>
>
> 2017-02-21 Segher Boessenkool <segher@kernel.crashing.org>
>
> * config/nios2/nios2.c (nios2_simple_const_p): Returns false if the
> argument isn't a CONST_INT.
> (nios2_alternate_compare_const): Set *alt_code and *alt_op to code
> and op if op is not a CONST_INT.
> (nios2_valid_compare_const_p): Return false if the argument isn't
> a CONST_INT.
> (ldstwm_operation_p): Return false if first_base is not a REG or
> if first_offset is not a CONST_INT.
OK.
jeff
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 4/6] nios2: Fixes for RTL checking
2017-02-21 14:49 ` [PATCH 4/6] nios2: Fixes " Segher Boessenkool
2017-02-21 19:33 ` Jeff Law
@ 2017-02-21 19:53 ` Sandra Loosemore
2017-02-24 22:45 ` Sandra Loosemore
1 sibling, 1 reply; 27+ messages in thread
From: Sandra Loosemore @ 2017-02-21 19:53 UTC (permalink / raw)
To: Segher Boessenkool, gcc-patches; +Cc: bschmidt
On 02/21/2017 07:48 AM, Segher Boessenkool wrote:
> You cannot call REGNO on something that isn't a REG, and you cannot
> call INTVAL on something that isn't a CONST_INT.
>
> The way I fixed nios2_alternate_compare_const is admittedly a bit lame.
Yeah. :-P
> 2017-02-21 Segher Boessenkool <segher@kernel.crashing.org>
>
> * config/nios2/nios2.c (nios2_simple_const_p): Returns false if the
> argument isn't a CONST_INT.
> (nios2_alternate_compare_const): Set *alt_code and *alt_op to code
> and op if op is not a CONST_INT.
> (nios2_valid_compare_const_p): Return false if the argument isn't
> a CONST_INT.
> (ldstwm_operation_p): Return false if first_base is not a REG or
> if first_offset is not a CONST_INT.
Give me a couple days to fiddle with this a bit and run regression tests.
-Sandra
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 4/6] nios2: Fixes for RTL checking
2017-02-21 19:53 ` Sandra Loosemore
@ 2017-02-24 22:45 ` Sandra Loosemore
0 siblings, 0 replies; 27+ messages in thread
From: Sandra Loosemore @ 2017-02-24 22:45 UTC (permalink / raw)
To: Segher Boessenkool, gcc-patches; +Cc: bschmidt
[-- Attachment #1: Type: text/plain, Size: 1302 bytes --]
On 02/21/2017 12:41 PM, Sandra Loosemore wrote:
> On 02/21/2017 07:48 AM, Segher Boessenkool wrote:
>> You cannot call REGNO on something that isn't a REG, and you cannot
>> call INTVAL on something that isn't a CONST_INT.
>>
>> The way I fixed nios2_alternate_compare_const is admittedly a bit lame.
>
> Yeah. :-P
>
>> 2017-02-21 Segher Boessenkool <segher@kernel.crashing.org>
>>
>> * config/nios2/nios2.c (nios2_simple_const_p): Returns false if the
>> argument isn't a CONST_INT.
>> (nios2_alternate_compare_const): Set *alt_code and *alt_op to code
>> and op if op is not a CONST_INT.
>> (nios2_valid_compare_const_p): Return false if the argument isn't
>> a CONST_INT.
>> (ldstwm_operation_p): Return false if first_base is not a REG or
>> if first_offset is not a CONST_INT.
>
> Give me a couple days to fiddle with this a bit and run regression tests.
>
> -Sandra
>
I've checked in the attached version of the patch. I basically moved
the CONST_INT test into nios2_validate_compare and changed its two
helper functions to assert if they get the wrong thing, instead of
trying to make them cope with other cases.
I regression-tested this on nios2-elf and did some tests on
nios2-linux-gnu as well.
Thanks, Segher, for identifying these bugs.
-Sandra
[-- Attachment #2: check.log --]
[-- Type: text/x-log, Size: 524 bytes --]
2017-02-24 Segher Boessenkool <segher@kernel.crashing.org>
Sandra Loosemore <sandra@codesourcery.com>
gcc/
* config/nios2/nios2.c (nios2_simple_const_p): Returns false if the
argument isn't a CONST_INT.
(nios2_alternate_compare_const): Assert op is a CONST_INT.
(nios2_valid_compare_const_p): Assert op is a CONST_INT.
(nios2_validate_compare): Bypass alternate compare logic if *op2
is not a CONST_INT.
(ldstwm_operation_p): Return false if first_base is not a REG or
if first_offset is not a CONST_INT.
[-- Attachment #3: check.patch --]
[-- Type: text/x-patch, Size: 2096 bytes --]
Index: gcc/config/nios2/nios2.c
===================================================================
--- gcc/config/nios2/nios2.c (revision 245646)
+++ gcc/config/nios2/nios2.c (working copy)
@@ -1416,6 +1416,8 @@ nios2_option_override (void)
static bool
nios2_simple_const_p (const_rtx cst)
{
+ if (!CONST_INT_P (cst))
+ return false;
HOST_WIDE_INT val = INTVAL (cst);
return SMALL_INT (val) || SMALL_INT_UNSIGNED (val) || UPPER16_INT (val);
}
@@ -1753,6 +1755,8 @@ nios2_alternate_compare_const (enum rtx_
enum rtx_code *alt_code, rtx *alt_op,
machine_mode mode)
{
+ gcc_assert (CONST_INT_P (op));
+
HOST_WIDE_INT opval = INTVAL (op);
enum rtx_code scode = signed_condition (code);
bool dec_p = (scode == LT || scode == GE);
@@ -1788,6 +1792,7 @@ nios2_alternate_compare_const (enum rtx_
static bool
nios2_valid_compare_const_p (enum rtx_code code, rtx op)
{
+ gcc_assert (CONST_INT_P (op));
switch (code)
{
case EQ: case NE: case GE: case LT:
@@ -1846,7 +1851,7 @@ nios2_validate_compare (machine_mode mod
if (GET_MODE_CLASS (mode) == MODE_FLOAT)
return nios2_validate_fpu_compare (mode, cmp, op1, op2, true);
- if (!reg_or_0_operand (*op2, mode))
+ if (CONST_INT_P (*op2) && *op2 != const0_rtx)
{
/* Create alternate constant compare. */
nios2_alternate_compare_const (code, *op2, &alt_code, &alt_op2, mode);
@@ -1878,8 +1883,11 @@ nios2_validate_compare (machine_mode mod
code = alt_code;
*op2 = alt_op2;
}
- *op2 = force_reg (SImode, *op2);
+ *op2 = force_reg (mode, *op2);
}
+ else if (!reg_or_0_operand (*op2, mode))
+ *op2 = force_reg (mode, *op2);
+
check_rebuild_cmp:
if (code == GT || code == GTU || code == LE || code == LEU)
{
@@ -4558,6 +4566,8 @@ ldstwm_operation_p (rtx op, bool load_p)
if (!split_mem_address (XEXP (mem, 0),
&first_base, &first_offset))
return false;
+ if (!REG_P (first_base) || !CONST_INT_P (first_offset))
+ return false;
base_reg = first_base;
inc_p = INTVAL (first_offset) >= 0;
}
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/6] microblaze: Fixes for RTL checking
2017-02-21 14:48 [PATCH 0/6] Fallout from RTL checking Segher Boessenkool
` (2 preceding siblings ...)
2017-02-21 14:49 ` [PATCH 4/6] nios2: Fixes " Segher Boessenkool
@ 2017-02-21 14:49 ` Segher Boessenkool
2017-02-21 19:28 ` Jeff Law
` (2 more replies)
2017-02-21 14:49 ` [PATCH 6/6] sh: " Segher Boessenkool
` (2 subsequent siblings)
6 siblings, 3 replies; 27+ messages in thread
From: Segher Boessenkool @ 2017-02-21 14:49 UTC (permalink / raw)
To: gcc-patches; +Cc: Segher Boessenkool, eager
REGNO can only be called on REGs, not SUBREGs; and INTVAL does not work
on REGs.
2017-02-21 Segher Boessenkool <segher@kernel.crashing.org>
* config/microblaze/microblaze.c (microblaze_expand_shift): Do not
test for register moves to themselves.
* config/microblaze/microblaze.md (*ashlsi3_byone, *ashrsi3_byone,
*lshrsi3_byone): Test for const1_rtx instead of calling INTVAL on
something that isn't necessarily a CONST_INT.
---
gcc/config/microblaze/microblaze.c | 5 ++---
gcc/config/microblaze/microblaze.md | 6 +++---
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/gcc/config/microblaze/microblaze.c b/gcc/config/microblaze/microblaze.c
index 746bef1..4850e85 100644
--- a/gcc/config/microblaze/microblaze.c
+++ b/gcc/config/microblaze/microblaze.c
@@ -3322,11 +3322,10 @@ microblaze_expand_shift (rtx operands[])
|| (GET_CODE (operands[1]) == REG)
|| (GET_CODE (operands[1]) == SUBREG));
- /* Shift by zero -- copy regs if necessary. */
+ /* Shift by zero -- copy regs. */
if ((GET_CODE (operands[2]) == CONST_INT) && (INTVAL (operands[2]) == 0))
{
- if (REGNO (operands[0]) != REGNO (operands[1]))
- emit_insn (gen_movsi (operands[0], operands[1]));
+ emit_insn (gen_movsi (operands[0], operands[1]));
return 1;
}
diff --git a/gcc/config/microblaze/microblaze.md b/gcc/config/microblaze/microblaze.md
index 66ebc1e..9cf83f5 100644
--- a/gcc/config/microblaze/microblaze.md
+++ b/gcc/config/microblaze/microblaze.md
@@ -1321,7 +1321,7 @@ (define_insn "*ashlsi3_byone"
[(set (match_operand:SI 0 "register_operand" "=d")
(ashift:SI (match_operand:SI 1 "register_operand" "d")
(match_operand:SI 2 "arith_operand" "I")))]
- "(INTVAL (operands[2]) == 1)"
+ "operands[2] == const1_rtx"
"addk\t%0,%1,%1"
[(set_attr "type" "arith")
(set_attr "mode" "SI")
@@ -1482,7 +1482,7 @@ (define_insn "*ashrsi3_byone"
[(set (match_operand:SI 0 "register_operand" "=d")
(ashiftrt:SI (match_operand:SI 1 "register_operand" "d")
(match_operand:SI 2 "arith_operand" "I")))]
- "(INTVAL (operands[2]) == 1)"
+ "operands[2] == const1_rtx"
"sra\t%0,%1"
[(set_attr "type" "arith")
(set_attr "mode" "SI")
@@ -1571,7 +1571,7 @@ (define_insn "*lshrsi3_byone"
[(set (match_operand:SI 0 "register_operand" "=d")
(lshiftrt:SI (match_operand:SI 1 "register_operand" "d")
(match_operand:SI 2 "arith_operand" "I")))]
- "(INTVAL (operands[2]) == 1)"
+ "operands[2] == const1_rtx"
"srl\t%0,%1"
[(set_attr "type" "arith")
(set_attr "mode" "SI")
--
1.9.3
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 3/6] microblaze: Fixes for RTL checking
2017-02-21 14:49 ` [PATCH 3/6] microblaze: " Segher Boessenkool
@ 2017-02-21 19:28 ` Jeff Law
2017-02-21 20:11 ` Michael Eager
2017-02-21 20:19 ` Jakub Jelinek
2 siblings, 0 replies; 27+ messages in thread
From: Jeff Law @ 2017-02-21 19:28 UTC (permalink / raw)
To: Segher Boessenkool, gcc-patches; +Cc: eager
On 02/21/2017 07:48 AM, Segher Boessenkool wrote:
> REGNO can only be called on REGs, not SUBREGs; and INTVAL does not work
> on REGs.
>
> 2017-02-21 Segher Boessenkool <segher@kernel.crashing.org>
>
> * config/microblaze/microblaze.c (microblaze_expand_shift): Do not
> test for register moves to themselves.
> * config/microblaze/microblaze.md (*ashlsi3_byone, *ashrsi3_byone,
> *lshrsi3_byone): Test for const1_rtx instead of calling INTVAL on
> something that isn't necessarily a CONST_INT.
So I wanted to make sure that the avoidance of a nop-move resulting from
a nop-shift wasn't because the ISA couldn't encode a nop-move (and yes,
I've run into such ISAs).
The microblaze doesn't seem to have any trouble encoding a nop move, so
if we were to generate one due to this change it won't cause a problem.
OK for the trunk.
Jeff
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/6] microblaze: Fixes for RTL checking
2017-02-21 14:49 ` [PATCH 3/6] microblaze: " Segher Boessenkool
2017-02-21 19:28 ` Jeff Law
@ 2017-02-21 20:11 ` Michael Eager
2017-02-21 20:33 ` Segher Boessenkool
2017-02-21 20:19 ` Jakub Jelinek
2 siblings, 1 reply; 27+ messages in thread
From: Michael Eager @ 2017-02-21 20:11 UTC (permalink / raw)
To: segher; +Cc: gcc-patches
On 02/21/2017 06:48 AM, Segher Boessenkool wrote:
> REGNO can only be called on REGs, not SUBREGs; and INTVAL does not work
> on REGs.
>
> 2017-02-21 Segher Boessenkool <segher@kernel.crashing.org>
>
> * config/microblaze/microblaze.c (microblaze_expand_shift): Do not
> test for register moves to themselves.
> * config/microblaze/microblaze.md (*ashlsi3_byone, *ashrsi3_byone,
> *lshrsi3_byone): Test for const1_rtx instead of calling INTVAL on
> something that isn't necessarily a CONST_INT.
>
> ---
> gcc/config/microblaze/microblaze.c | 5 ++---
> gcc/config/microblaze/microblaze.md | 6 +++---
> 2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/config/microblaze/microblaze.c b/gcc/config/microblaze/microblaze.c
> index 746bef1..4850e85 100644
> --- a/gcc/config/microblaze/microblaze.c
> +++ b/gcc/config/microblaze/microblaze.c
> @@ -3322,11 +3322,10 @@ microblaze_expand_shift (rtx operands[])
> || (GET_CODE (operands[1]) == REG)
> || (GET_CODE (operands[1]) == SUBREG));
>
> - /* Shift by zero -- copy regs if necessary. */
> + /* Shift by zero -- copy regs. */
> if ((GET_CODE (operands[2]) == CONST_INT) && (INTVAL (operands[2]) == 0))
> {
> - if (REGNO (operands[0]) != REGNO (operands[1]))
> - emit_insn (gen_movsi (operands[0], operands[1]));
> + emit_insn (gen_movsi (operands[0], operands[1]));
> return 1;
> }
Why generate an unnecessary NOP?
--
Michael Eager eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306 650-325-8077
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 3/6] microblaze: Fixes for RTL checking
2017-02-21 20:11 ` Michael Eager
@ 2017-02-21 20:33 ` Segher Boessenkool
2017-02-21 21:07 ` Michael Eager
0 siblings, 1 reply; 27+ messages in thread
From: Segher Boessenkool @ 2017-02-21 20:33 UTC (permalink / raw)
To: Michael Eager; +Cc: gcc-patches
On Tue, Feb 21, 2017 at 12:08:34PM -0800, Michael Eager wrote:
> >- /* Shift by zero -- copy regs if necessary. */
> >+ /* Shift by zero -- copy regs. */
> > if ((GET_CODE (operands[2]) == CONST_INT) && (INTVAL (operands[2]) ==
> > 0))
> > {
> >- if (REGNO (operands[0]) != REGNO (operands[1]))
> >- emit_insn (gen_movsi (operands[0], operands[1]));
> >+ emit_insn (gen_movsi (operands[0], operands[1]));
> > return 1;
> > }
>
> Why generate an unnecessary NOP?
Why not? It will be optimised away anyway, and the code to get at the
subregs is hairy... But could optimise away the useless move here
already if both ops are a reg and both are the same, if you prefer that?
Or rtx_equal_p (operands[0], operands[1])?
Segher
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 3/6] microblaze: Fixes for RTL checking
2017-02-21 20:33 ` Segher Boessenkool
@ 2017-02-21 21:07 ` Michael Eager
2017-02-24 23:12 ` Segher Boessenkool
0 siblings, 1 reply; 27+ messages in thread
From: Michael Eager @ 2017-02-21 21:07 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches, Jakub Jelinek
On 02/21/2017 12:25 PM, Segher Boessenkool wrote:
> On Tue, Feb 21, 2017 at 12:08:34PM -0800, Michael Eager wrote:
>>> - /* Shift by zero -- copy regs if necessary. */
>>> + /* Shift by zero -- copy regs. */
>>> if ((GET_CODE (operands[2]) == CONST_INT) && (INTVAL (operands[2]) ==
>>> 0))
>>> {
>>> - if (REGNO (operands[0]) != REGNO (operands[1]))
>>> - emit_insn (gen_movsi (operands[0], operands[1]));
>>> + emit_insn (gen_movsi (operands[0], operands[1]));
>>> return 1;
>>> }
>>
>> Why generate an unnecessary NOP?
>
> Why not? It will be optimised away anyway, and the code to get at the
> subregs is hairy... But could optimise away the useless move here
> already if both ops are a reg and both are the same, if you prefer that?
> Or rtx_equal_p (operands[0], operands[1])?
It's optimized away only if optimization is on.
The existing code checks and does not generate the unneeded move.
Whatever you change to clean up code should maintain the same behavior.
if ((operands[2] == const0_rtx) && !rtx_equal_p (operands[0], operands[1]))
{
emit_insn (gen_movsi (operands[0], operands[1]));
}
--
Michael Eager eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306 650-325-8077
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 3/6] microblaze: Fixes for RTL checking
2017-02-21 21:07 ` Michael Eager
@ 2017-02-24 23:12 ` Segher Boessenkool
0 siblings, 0 replies; 27+ messages in thread
From: Segher Boessenkool @ 2017-02-24 23:12 UTC (permalink / raw)
To: Michael Eager; +Cc: gcc-patches, Jakub Jelinek
On Tue, Feb 21, 2017 at 01:02:30PM -0800, Michael Eager wrote:
> >>Why generate an unnecessary NOP?
> >
> >Why not? It will be optimised away anyway, and the code to get at the
> >subregs is hairy... But could optimise away the useless move here
> >already if both ops are a reg and both are the same, if you prefer that?
> >Or rtx_equal_p (operands[0], operands[1])?
>
> It's optimized away only if optimization is on.
>
> The existing code checks and does not generate the unneeded move.
> Whatever you change to clean up code should maintain the same behavior.
>
> if ((operands[2] == const0_rtx) && !rtx_equal_p (operands[0], operands[1]))
> {
> emit_insn (gen_movsi (operands[0], operands[1]));
> }
Hi Michael,
Will you take care of it?
Segher
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/6] microblaze: Fixes for RTL checking
2017-02-21 14:49 ` [PATCH 3/6] microblaze: " Segher Boessenkool
2017-02-21 19:28 ` Jeff Law
2017-02-21 20:11 ` Michael Eager
@ 2017-02-21 20:19 ` Jakub Jelinek
2017-02-21 20:37 ` Michael Eager
2 siblings, 1 reply; 27+ messages in thread
From: Jakub Jelinek @ 2017-02-21 20:19 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: gcc-patches, eager
On Tue, Feb 21, 2017 at 02:48:15PM +0000, Segher Boessenkool wrote:
> - /* Shift by zero -- copy regs if necessary. */
> + /* Shift by zero -- copy regs. */
> if ((GET_CODE (operands[2]) == CONST_INT) && (INTVAL (operands[2]) == 0))
You could have changed this line to
if (operands[2] == const0_rtx)
as well.
Jakub
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/6] microblaze: Fixes for RTL checking
2017-02-21 20:19 ` Jakub Jelinek
@ 2017-02-21 20:37 ` Michael Eager
2017-02-21 20:41 ` Jakub Jelinek
2017-02-21 21:23 ` Segher Boessenkool
0 siblings, 2 replies; 27+ messages in thread
From: Michael Eager @ 2017-02-21 20:37 UTC (permalink / raw)
To: Jakub Jelinek, Segher Boessenkool; +Cc: gcc-patches
On 02/21/2017 12:15 PM, Jakub Jelinek wrote:
> On Tue, Feb 21, 2017 at 02:48:15PM +0000, Segher Boessenkool wrote:
>> - /* Shift by zero -- copy regs if necessary. */
>> + /* Shift by zero -- copy regs. */
>> if ((GET_CODE (operands[2]) == CONST_INT) && (INTVAL (operands[2]) == 0))
>
> You could have changed this line to
> if (operands[2] == const0_rtx)
> as well.
And this would not change the generated code.
--
Michael Eager eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306 650-325-8077
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/6] microblaze: Fixes for RTL checking
2017-02-21 20:37 ` Michael Eager
@ 2017-02-21 20:41 ` Jakub Jelinek
2017-02-21 21:23 ` Segher Boessenkool
1 sibling, 0 replies; 27+ messages in thread
From: Jakub Jelinek @ 2017-02-21 20:41 UTC (permalink / raw)
To: Michael Eager; +Cc: Segher Boessenkool, gcc-patches
On Tue, Feb 21, 2017 at 12:35:17PM -0800, Michael Eager wrote:
> On 02/21/2017 12:15 PM, Jakub Jelinek wrote:
> > On Tue, Feb 21, 2017 at 02:48:15PM +0000, Segher Boessenkool wrote:
> > > - /* Shift by zero -- copy regs if necessary. */
> > > + /* Shift by zero -- copy regs. */
> > > if ((GET_CODE (operands[2]) == CONST_INT) && (INTVAL (operands[2]) == 0))
> >
> > You could have changed this line to
> > if (operands[2] == const0_rtx)
> > as well.
>
> And this would not change the generated code.
Sure, and it doesn't need to be done for GCC7, but would be a nice cleanup
for stage1 (e.g. remove redundant parens in the backend, cases like this
where because of the uniqueness of CONST_INT values a pointer comparison
is enough, or using macros where available (e.g. GET_CODE (operands[2]) == CONST_INT
can be written as CONST_INT_P (operands[2]), etc.).
Jakub
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/6] microblaze: Fixes for RTL checking
2017-02-21 20:37 ` Michael Eager
2017-02-21 20:41 ` Jakub Jelinek
@ 2017-02-21 21:23 ` Segher Boessenkool
1 sibling, 0 replies; 27+ messages in thread
From: Segher Boessenkool @ 2017-02-21 21:23 UTC (permalink / raw)
To: Michael Eager; +Cc: Jakub Jelinek, gcc-patches
On Tue, Feb 21, 2017 at 12:35:17PM -0800, Michael Eager wrote:
> On 02/21/2017 12:15 PM, Jakub Jelinek wrote:
> >On Tue, Feb 21, 2017 at 02:48:15PM +0000, Segher Boessenkool wrote:
> >>- /* Shift by zero -- copy regs if necessary. */
> >>+ /* Shift by zero -- copy regs. */
> >> if ((GET_CODE (operands[2]) == CONST_INT) && (INTVAL (operands[2]) ==
> >> 0))
> >
> >You could have changed this line to
> > if (operands[2] == const0_rtx)
> >as well.
>
> And this would not change the generated code.
Of course, but you still need the other changes.
I did not make this random cleanup because a) it is a random cleanup; and
b) there are at least three occurrences of this in both microblaze.c and
microblaze.md .
Michael, will you make a fix yourself? --enable-checking=yes,rtl will
reproduce the problem.
Segher
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/6] sh: Fixes for RTL checking
2017-02-21 14:48 [PATCH 0/6] Fallout from RTL checking Segher Boessenkool
` (3 preceding siblings ...)
2017-02-21 14:49 ` [PATCH 3/6] microblaze: " Segher Boessenkool
@ 2017-02-21 14:49 ` Segher Boessenkool
2017-02-21 15:49 ` Oleg Endo
2017-02-21 15:01 ` [PATCH 5/6] pa: " Segher Boessenkool
2017-02-21 15:16 ` [PATCH] arc: " Segher Boessenkool
6 siblings, 1 reply; 27+ messages in thread
From: Segher Boessenkool @ 2017-02-21 14:49 UTC (permalink / raw)
To: gcc-patches; +Cc: Segher Boessenkool, aoliva, kkojima, olegendo
2017-02-21 Segher Boessenkool <segher@kernel.crashing.org>
* config/sh/sh.md (tstsi_t): If operands[0] is a SUBREG instead of
a REG, look at the REG it is a SUBREG of.
(splitter for cmpeqsi_t): Ditto.
---
gcc/config/sh/sh.md | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/gcc/config/sh/sh.md b/gcc/config/sh/sh.md
index 2645fca..e19e977 100644
--- a/gcc/config/sh/sh.md
+++ b/gcc/config/sh/sh.md
@@ -561,8 +561,12 @@ (define_insn_and_split "tstsi_t"
gcc_assert (CONST_INT_P (operands[1]));
HOST_WIDE_INT op1val = INTVAL (operands[1]);
+ rtx reg = operands[0];
+ if (SUBREG_P (reg))
+ reg = SUBREG_REG (reg);
+ gcc_assert (REG_P (reg));
bool op0_dead_after_this =
- sh_reg_dead_or_unused_after_insn (curr_insn, REGNO (operands[0]));
+ sh_reg_dead_or_unused_after_insn (curr_insn, REGNO (reg));
if (optimize)
{
@@ -834,7 +838,11 @@ (define_split
/* If the tested reg is not dead after this insn, it's probably used by
something else after the comparison. It's probably better to leave
it as it is. */
- if (find_regno_note (curr_insn, REG_DEAD, REGNO (operands[0])) == NULL_RTX)
+ rtx reg = operands[0];
+ if (SUBREG_P (reg))
+ reg = SUBREG_REG (reg);
+ gcc_assert (REG_P (reg));
+ if (find_regno_note (curr_insn, REG_DEAD, REGNO (reg)) != NULL_RTX)
FAIL;
/* FIXME: Maybe also search the predecessor basic blocks to catch
--
1.9.3
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 6/6] sh: Fixes for RTL checking
2017-02-21 14:49 ` [PATCH 6/6] sh: " Segher Boessenkool
@ 2017-02-21 15:49 ` Oleg Endo
2017-02-21 16:32 ` Segher Boessenkool
0 siblings, 1 reply; 27+ messages in thread
From: Oleg Endo @ 2017-02-21 15:49 UTC (permalink / raw)
To: Segher Boessenkool, gcc-patches; +Cc: aoliva, kkojima, olegendo
On Tue, 2017-02-21 at 14:48 +0000, Segher Boessenkool wrote:
> 2017-02-21  Segher Boessenkool  <segher@kernel.crashing.org>
>
> * config/sh/sh.md (tstsi_t): If operands[0] is a SUBREG instead of
> a REG, look at the REG it is a SUBREG of.
> (splitter for cmpeqsi_t): Ditto.
>
> ---
> Â gcc/config/sh/sh.md | 12 ++++++++++--
> Â 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/config/sh/sh.md b/gcc/config/sh/sh.md
> index 2645fca..e19e977 100644
> --- a/gcc/config/sh/sh.md
> +++ b/gcc/config/sh/sh.md
> @@ -561,8 +561,12 @@ (define_insn_and_split "tstsi_t"
> Â Â Â gcc_assert (CONST_INT_P (operands[1]));
> Â
> Â Â Â HOST_WIDE_INT op1val = INTVAL (operands[1]);
> +Â Â rtx reg = operands[0];
> +Â Â if (SUBREG_P (reg))
> +Â Â Â Â reg = SUBREG_REG (reg);
> +Â Â gcc_assert (REG_P (reg));
> Â Â Â bool op0_dead_after_this =
> - sh_reg_dead_or_unused_after_insn (curr_insn, REGNO
> (operands[0]));
> + sh_reg_dead_or_unused_after_insn (curr_insn, REGNO (reg));
> Â
Thanks for dealing with those.
That SUBREG vs. REG stuff is annoying. Â Isn't there a simple function
that just does the right thing which can be used instead of manually
open-coding these checks over and over again?
Cheers,
Oleg
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 6/6] sh: Fixes for RTL checking
2017-02-21 15:49 ` Oleg Endo
@ 2017-02-21 16:32 ` Segher Boessenkool
0 siblings, 0 replies; 27+ messages in thread
From: Segher Boessenkool @ 2017-02-21 16:32 UTC (permalink / raw)
To: Oleg Endo; +Cc: gcc-patches, aoliva, kkojima, olegendo
On Wed, Feb 22, 2017 at 12:16:43AM +0900, Oleg Endo wrote:
> That SUBREG vs. REG stuff is annoying. Â Isn't there a simple function
> that just does the right thing which can be used instead of manually
> open-coding these checks over and over again?
There is reg_or_subregno, which would even work here :-)
Segher
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 5/6] pa: Fixes for RTL checking
2017-02-21 14:48 [PATCH 0/6] Fallout from RTL checking Segher Boessenkool
` (4 preceding siblings ...)
2017-02-21 14:49 ` [PATCH 6/6] sh: " Segher Boessenkool
@ 2017-02-21 15:01 ` Segher Boessenkool
2017-02-21 19:12 ` Jeff Law
2017-02-21 15:16 ` [PATCH] arc: " Segher Boessenkool
6 siblings, 1 reply; 27+ messages in thread
From: Segher Boessenkool @ 2017-02-21 15:01 UTC (permalink / raw)
To: gcc-patches; +Cc: Segher Boessenkool, law, dave.anglin
I do not know what the USEs are really for, so I do not know if using
just the pattern here is correct. Using the full insn is probably not
correct either though.
2017-02-21 Segher Boessenkool <segher@kernel.crashing.org>
* config/pa/pa.c (pa_combine_instructions): Do not share RTL. Make
the special USEs with the pattern of the insn, not the insn itself.
---
gcc/config/pa/pa.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/gcc/config/pa/pa.c b/gcc/config/pa/pa.c
index b0b3311..3f7b2c7 100644
--- a/gcc/config/pa/pa.c
+++ b/gcc/config/pa/pa.c
@@ -9178,17 +9178,17 @@ pa_combine_instructions (void)
|| anchor_attr == PA_COMBINE_TYPE_FMPY))
{
/* Emit the new instruction and delete the old anchor. */
- emit_insn_before (gen_rtx_PARALLEL
- (VOIDmode,
- gen_rtvec (2, PATTERN (anchor),
- PATTERN (floater))),
- anchor);
+ rtvec vtemp = gen_rtvec (2, copy_rtx (PATTERN (anchor)),
+ copy_rtx (PATTERN (floater)));
+ rtx temp = gen_rtx_PARALLEL (VOIDmode, vtemp);
+ emit_insn_before (temp, anchor);
SET_INSN_DELETED (anchor);
/* Emit a special USE insn for FLOATER, then delete
the floating insn. */
- emit_insn_before (gen_rtx_USE (VOIDmode, floater), floater);
+ temp = copy_rtx (PATTERN (floater));
+ emit_insn_before (gen_rtx_USE (VOIDmode, temp), floater);
delete_insn (floater);
continue;
@@ -9196,21 +9196,19 @@ pa_combine_instructions (void)
else if (floater
&& anchor_attr == PA_COMBINE_TYPE_UNCOND_BRANCH)
{
- rtx temp;
/* Emit the new_jump instruction and delete the old anchor. */
- temp
- = emit_jump_insn_before (gen_rtx_PARALLEL
- (VOIDmode,
- gen_rtvec (2, PATTERN (anchor),
- PATTERN (floater))),
- anchor);
+ rtvec vtemp = gen_rtvec (2, copy_rtx (PATTERN (anchor)),
+ copy_rtx (PATTERN (floater)));
+ rtx temp = gen_rtx_PARALLEL (VOIDmode, vtemp);
+ temp = emit_jump_insn_before (temp, anchor);
JUMP_LABEL (temp) = JUMP_LABEL (anchor);
SET_INSN_DELETED (anchor);
/* Emit a special USE insn for FLOATER, then delete
the floating insn. */
- emit_insn_before (gen_rtx_USE (VOIDmode, floater), floater);
+ temp = copy_rtx (PATTERN (floater));
+ emit_insn_before (gen_rtx_USE (VOIDmode, temp), floater);
delete_insn (floater);
continue;
}
--
1.9.3
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 5/6] pa: Fixes for RTL checking
2017-02-21 15:01 ` [PATCH 5/6] pa: " Segher Boessenkool
@ 2017-02-21 19:12 ` Jeff Law
0 siblings, 0 replies; 27+ messages in thread
From: Jeff Law @ 2017-02-21 19:12 UTC (permalink / raw)
To: Segher Boessenkool, gcc-patches; +Cc: dave.anglin
On 02/21/2017 07:48 AM, Segher Boessenkool wrote:
> I do not know what the USEs are really for, so I do not know if using
> just the pattern here is correct. Using the full insn is probably not
> correct either though.
>
>
> 2017-02-21 Segher Boessenkool <segher@kernel.crashing.org>
>
> * config/pa/pa.c (pa_combine_instructions): Do not share RTL. Make
> the special USEs with the pattern of the insn, not the insn itself.
OK. Or alternately, remove all that code :-) The optimization it was
doing was only useful on pa7xx/pa7xxx models which are beyond ancient.
Jeff
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH] arc: Fixes for RTL checking
2017-02-21 14:48 [PATCH 0/6] Fallout from RTL checking Segher Boessenkool
` (5 preceding siblings ...)
2017-02-21 15:01 ` [PATCH 5/6] pa: " Segher Boessenkool
@ 2017-02-21 15:16 ` Segher Boessenkool
2017-02-21 19:27 ` Jeff Law
2017-02-24 19:20 ` Claudiu Zissulescu
6 siblings, 2 replies; 27+ messages in thread
From: Segher Boessenkool @ 2017-02-21 15:16 UTC (permalink / raw)
To: gcc-patches; +Cc: gnu, segher
Whoops, I forgot one. Here it is. Joern, please see
<https://gcc.gnu.org/ml/gcc-patches/2017-02/msg01291.html>.
2017-02-21 Segher Boessenkool <segher@kernel.crashing.org>
* config/arc/arc.c (arc_ccfsm_advance): Only take the PATTERN of
this_insn if it is an INSN or JUMP_INSN.
(force_offsettable): Look at base, not at addr.
* config/arc/predicates.md (brcc_nolimm_operator): Don't call INTVAL
on things that aren' necessarily CONST_INTs.
---
gcc/config/arc/arc.c | 8 +++++---
gcc/config/arc/predicates.md | 2 ++
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/gcc/config/arc/arc.c b/gcc/config/arc/arc.c
index 8a838f9..4c99f1d 100644
--- a/gcc/config/arc/arc.c
+++ b/gcc/config/arc/arc.c
@@ -3832,8 +3832,6 @@ arc_ccfsm_advance (rtx_insn *insn, struct arc_ccfsm *state)
break;
}
- scanbody = PATTERN (this_insn);
-
switch (GET_CODE (this_insn))
{
case CODE_LABEL:
@@ -3868,6 +3866,8 @@ arc_ccfsm_advance (rtx_insn *insn, struct arc_ccfsm *state)
break;
case JUMP_INSN:
+ scanbody = PATTERN (this_insn);
+
/* If this is an unconditional branch to the same label, succeed.
If it is to another label, do nothing. If it is conditional,
fail. */
@@ -3902,6 +3902,8 @@ arc_ccfsm_advance (rtx_insn *insn, struct arc_ccfsm *state)
break;
case INSN:
+ scanbody = PATTERN (this_insn);
+
/* We can only do this with insns that can use the condition
codes (and don't set them). */
if (GET_CODE (scanbody) == SET
@@ -7401,7 +7403,7 @@ force_offsettable (rtx addr, HOST_WIDE_INT size, bool reuse)
}
if (!REG_P (base)
|| (REGNO (base) != STACK_POINTER_REGNUM
- && REGNO_PTR_FRAME_P (REGNO (addr)))
+ && REGNO_PTR_FRAME_P (REGNO (base)))
|| !CONST_INT_P (offs) || !SMALL_INT (INTVAL (offs))
|| !SMALL_INT (INTVAL (offs) + size))
{
diff --git a/gcc/config/arc/predicates.md b/gcc/config/arc/predicates.md
index 159a6b4..0dec736 100644
--- a/gcc/config/arc/predicates.md
+++ b/gcc/config/arc/predicates.md
@@ -458,8 +458,10 @@ (define_predicate "ge_lt_comparison_operator"
(define_predicate "brcc_nolimm_operator"
(ior (match_test "REG_P (XEXP (op, 1))")
(and (match_code "eq, ne, lt, ge, ltu, geu")
+ (match_test "CONST_INT_P (XEXP (op, 1))")
(match_test "u6_immediate_operand (XEXP (op, 1), SImode)"))
(and (match_code "le, gt, leu, gtu")
+ (match_test "CONST_INT_P (XEXP (op, 1))")
(match_test "UNSIGNED_INT6 (INTVAL (XEXP (op, 1)) + 1)"))))
;; Return TRUE if this is the condition code register, if we aren't given
--
1.9.3
^ permalink raw reply [flat|nested] 27+ messages in thread