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

2017-02-24 Thread 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

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

2017-02-21 Thread 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 +, 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 3/6] microblaze: Fixes for RTL checking

2017-02-21 Thread 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 Eagerea...@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

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

2017-02-21 Thread 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 +, 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

2017-02-21 Thread Michael Eager 

On 02/21/2017 12:15 PM, Jakub Jelinek wrote:

On Tue, Feb 21, 2017 at 02:48:15PM +, 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 Eagerea...@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

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

2017-02-21 Thread 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

2017-02-21 Thread Jakub Jelinek 
On Tue, Feb 21, 2017 at 02:48:15PM +, 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

2017-02-21 Thread 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  

* 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 Eagerea...@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077

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

2017-02-21 Thread 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  

* 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

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

2017-02-21 Thread Segher Boessenkool 
REGNO can only be called on REGs, not SUBREGs; and INTVAL does not work
on REGs.

2017-02-21  Segher Boessenkool  

* 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