[PATCH 0/6] Fallout from RTL checking

[PATCH 0/6] Fallout from RTL checking

[Segher Boessenkool]

Hi!

I tested building a cross compiler to all Linux targets (well, those
that are supported by GCC), and building Linux with that.  I used
--enable-checking=yes,rtl,tree and there was some fallout; all in
ports, and all with RTL checking.

c6x and pa illegally share RTL.  The rest of the failures are calling
REGNO on something that isn't a REG, and INTVAL on something that isn't
a CONST_INT.

These patches were tested as described (not bootstrapped and/or
regression checked).  Are they okay to commit, or do you want to handle
things differently?


Segher


Segher Boessenkool (6):
  c6x: Fix for RTL checking
  cris: Fix for RTL checking
  microblaze: Fixes for RTL checking
  nios2: Fixes for RTL checking
  pa: Fixes for RTL checking
  sh: Fixes for RTL checking

 gcc/config/c6x/c6x.c                |  1 +
 gcc/config/cris/cris.md             |  2 +-
 gcc/config/microblaze/microblaze.c  |  5 ++---
 gcc/config/microblaze/microblaze.md |  6 +++---
 gcc/config/nios2/nios2.c            | 14 ++++++++++++++
 gcc/config/pa/pa.c                  | 26 ++++++++++++--------------
 gcc/config/sh/sh.md                 | 12 ++++++++++--
 7 files changed, 43 insertions(+), 23 deletions(-)

-- 
1.9.3

[PATCH 2/6] cris: Fix for RTL checking

[Segher Boessenkool]

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

[PATCH 3/6] microblaze: Fixes for RTL checking

[Segher Boessenkool]

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

[PATCH 4/6] nios2: Fixes for RTL checking

[Segher Boessenkool]

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

[PATCH 6/6] sh: Fixes for RTL checking

[Segher Boessenkool]

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

[PATCH 1/6] c6x: Fix for RTL checking

[Segher Boessenkool]

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

[PATCH 5/6] pa: Fixes for RTL checking

[Segher Boessenkool]

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

Re: [PATCH 2/6] cris: Fix for RTL checking

[Hans-Peter Nilsson]

> 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

Re: [PATCH 1/6] c6x: Fix for RTL checking

[Bernd Schmidt]

On 02/21/2017 03:48 PM, Segher Boessenkool wrote:
> 2017-02-21  Segher Boessenkool  <segher@kernel.crashing.org>
>
> 	* config/c6x/c6x.c (predicate_insn): Do not incorrectly share RTL.

Ok, thanks.


Bernd

[PATCH] arc: Fixes for RTL checking

[Segher Boessenkool]

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

Re: [PATCH 6/6] sh: Fixes for RTL checking

[Oleg Endo]

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

Re: [PATCH 6/6] sh: Fixes for RTL checking

[Segher Boessenkool]

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

Re: [PATCH 5/6] pa: Fixes for RTL checking

[Jeff Law]

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

Re: [PATCH] arc: Fixes for RTL checking

[Jeff Law]

On 02/21/2017 08:08 AM, Segher Boessenkool wrote:
> 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.
OK.
jeff

Re: [PATCH 3/6] microblaze: Fixes for RTL checking

[Jeff Law]

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

Re: [PATCH 4/6] nios2: Fixes for RTL checking

[Jeff Law]

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

Re: [PATCH 4/6] nios2: Fixes for RTL checking

[Sandra Loosemore]

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

Re: [PATCH 3/6] microblaze: Fixes for RTL checking

[Michael Eager]

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

Re: [PATCH 3/6] microblaze: Fixes for RTL checking

[Jakub Jelinek]

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

Re: [PATCH 3/6] microblaze: Fixes for RTL checking

[Segher Boessenkool]

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

Re: [PATCH 3/6] microblaze: Fixes for RTL checking

[Michael Eager]

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

Re: [PATCH 3/6] microblaze: Fixes for RTL checking

[Jakub Jelinek]

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

Re: [PATCH 3/6] microblaze: Fixes for RTL checking

[Michael Eager]

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

Re: [PATCH 3/6] microblaze: Fixes for RTL checking

[Segher Boessenkool]

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

Re: [PATCH] arc: Fixes for RTL checking

[Claudiu Zissulescu]

Thank you for ur contribution,
Claudiu

Re: [PATCH 4/6] nios2: Fixes for RTL checking

[Sandra Loosemore]

[-- 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;
 	}

Re: [PATCH 3/6] microblaze: Fixes for RTL checking

[Segher Boessenkool]

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