Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Jeff Law]

On 01/17/14 14:51, Jeff Law wrote:


Anyway, I clearly need to rethink that test.  Given this is something we
haven't seen in the wild, I'm going to disable it over the
weekend/monday so that enable-checking bugs pass and continue to ponder.

And the patch which disables the test.




jeff




commit 78f81be0894b38090cd6280f1e303610434d75c5
Author: Jeff Law 
Date:   Thu Jan 16 14:23:15 2014 -0700

   * ree.c (combine_set_extension): Temporarily disable test for
changing number of hard registers.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 37023c8..fabe408 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2014-01-17  Jeff Law  
+
+   * ree.c (combine_set_extension): Temporarily disable test for
+   changing number of hard registers.
+
 2014-01-17  Jan Hubicka  
 
PR middle-end/58125
diff --git a/gcc/ree.c b/gcc/ree.c
index 19d821c..421eb6c 100644
--- a/gcc/ree.c
+++ b/gcc/ree.c
@@ -297,11 +297,15 @@ combine_set_extension (ext_cand *cand, rtx curr_insn, rtx 
*orig_set)
   else
 new_reg = gen_rtx_REG (cand->mode, REGNO (SET_DEST (*orig_set)));
 
+#if 0
+  /* Rethinking test.  Temporarily disabled.  */
   /* We're going to be widening the result of DEF_INSN, ensure that doing so
  doesn't change the number of hard registers needed for the result.  */
   if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
-  != HARD_REGNO_NREGS (REGNO (orig_src), GET_MODE (SET_DEST (*orig_set
+  != HARD_REGNO_NREGS (REGNO (SET_DEST (*orig_set)),
+  GET_MODE (SET_DEST (*orig_set
return false;
+#endif
 
   /* Merge constants by directly moving the constant into the register under
  some conditions.  Recall that RTL constants are sign-extended.  */

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Jeff Law]

On 01/16/14 15:07, Jakub Jelinek wrote:

On Thu, Jan 16, 2014 at 02:31:09PM -0700, Jeff Law wrote:

+2014-01-16  Jeff Law  
+
+   * ree.c (combine_set_extension): Correct test for changing number
+   of hard registers when widening a reaching definition.
+
  2014-01-16  Bernd Schmidt  

PR middle-end/56791
diff --git a/gcc/ree.c b/gcc/ree.c
index 19d821c..96cddd2 100644
--- a/gcc/ree.c
+++ b/gcc/ree.c
@@ -300,7 +300,8 @@ combine_set_extension (ext_cand *cand, rtx curr_insn, rtx 
*orig_set)
/* We're going to be widening the result of DEF_INSN, ensure that doing so
   doesn't change the number of hard registers needed for the result.  */
if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
-  != HARD_REGNO_NREGS (REGNO (orig_src), GET_MODE (SET_DEST (*orig_set
+  != HARD_REGNO_NREGS (REGNO (SET_DEST (*orig_set)),
+  GET_MODE (SET_DEST (*orig_set


Shouldn't that be:
 if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
!= HARD_REGNO_NREGS (REGNO (new_reg), GET_MODE (SET_DEST (*orig_set
instead?

I mean, for the !copy_needed case it is obviously the same thing (and that
is what triggers in the testcase), but don't we generally want to check if
the same hard register in a wider mode will not occupy more registers, and
in particular the hard register we are considering to use on the lhs of the
defining insn (i.e. new_reg)?
I thought about using that conditional more than once.  But talked 
myself out of it every time on the grounds that I wanted to test the 
original destination REGNO of the reaching def.


Obviously that is REGNO (new_reg) if !copy_needed.  But it's something 
completely different if copy_needed.



In the copy_needed case there's actually two destinations to consider. 
 The original destination as well as the new destination.  Both will be 
set in a mode wider than the destination of the original reaching def. 
(one will be set in the modified reaching def and the other in a copy insn).


ISTM we need the # hard reg checked on the original destination as the 
other (upper) hard regs might be live across the sequence, but not 
used/set in the sequence.   Then we need some kind of check on the upper 
part of the new destination...  But I thought I covered that elsewhere...


Anyway, I clearly need to rethink that test.  Given this is something we 
haven't seen in the wild, I'm going to disable it over the 
weekend/monday so that enable-checking bugs pass and continue to ponder.


jeff

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Jeff Law]

On 01/17/14 01:41, Eric Botcazou wrote:

Bootstrapped & regression tested on x86_64-unknown-linux.  Also
bootstrapped with --enable-checking=rtl.


Note that you can do only one bootstrap with --enable-checking=yes,rtl.


Installed on the trunk.


As far as I can see, no, it was not installed.

Pilot error, it's not installed.

jefff

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Eric Botcazou]

> Bootstrapped & regression tested on x86_64-unknown-linux.  Also
> bootstrapped with --enable-checking=rtl.

Note that you can do only one bootstrap with --enable-checking=yes,rtl.

> Installed on the trunk.

As far as I can see, no, it was not installed.

-- 
Eric Botcazou

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Jakub Jelinek]

On Thu, Jan 16, 2014 at 02:31:09PM -0700, Jeff Law wrote:
> +2014-01-16  Jeff Law  
> +
> + * ree.c (combine_set_extension): Correct test for changing number
> + of hard registers when widening a reaching definition.
> +
>  2014-01-16  Bernd Schmidt  
>  
>   PR middle-end/56791
> diff --git a/gcc/ree.c b/gcc/ree.c
> index 19d821c..96cddd2 100644
> --- a/gcc/ree.c
> +++ b/gcc/ree.c
> @@ -300,7 +300,8 @@ combine_set_extension (ext_cand *cand, rtx curr_insn, rtx 
> *orig_set)
>/* We're going to be widening the result of DEF_INSN, ensure that doing so
>   doesn't change the number of hard registers needed for the result.  */
>if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
> -  != HARD_REGNO_NREGS (REGNO (orig_src), GET_MODE (SET_DEST 
> (*orig_set
> +  != HARD_REGNO_NREGS (REGNO (SET_DEST (*orig_set)),
> +GET_MODE (SET_DEST (*orig_set

Shouldn't that be:
if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
!= HARD_REGNO_NREGS (REGNO (new_reg), GET_MODE (SET_DEST (*orig_set
instead?

I mean, for the !copy_needed case it is obviously the same thing (and that
is what triggers in the testcase), but don't we generally want to check if
the same hard register in a wider mode will not occupy more registers, and
in particular the hard register we are considering to use on the lhs of the
defining insn (i.e. new_reg)?

Of course usually HARD_REGNO_NREGS will be the same for the same mode and
different GPR, so it would be really hard to create a testcase.

Jakub

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Jeff Law]

On 01/16/14 04:52, Eric Botcazou wrote:

Yes, like in the attached patch?  OK for the trunk?


Unfortunately this broke again bootstrap with RTL checking enabled on x86-64:

/home/eric/svn/gcc/libgcc/libgcc2.c: In function '__negdi2':
/home/eric/svn/gcc/libgcc/libgcc2.c:71:1: internal compiler error: RTL check:
expected code 'reg', have 'ne' in rhs_regno, at rtl.h:1125
Here's what I'm checking in.  No idea why I was looking at orig_src, 
that's just wrong.  We want to compare # hard regs for the original 
destination's regno/mode vs the # hard regs for the new destination's 
regno/mode.


Bootstrapped & regression tested on x86_64-unknown-linux.  Also 
bootstrapped with --enable-checking=rtl.


Installed on the trunk.


commit e9bbaae3fe335b8eef7022f684bc7c35926471ca
Author: Jeff Law 
Date:   Thu Jan 16 14:23:15 2014 -0700

   * ree.c (combine_set_extension): Correct test for changing number
of hard registers when widening a reaching definition.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index e07d1ae..3fa6f5f 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2014-01-16  Jeff Law  
+
+   * ree.c (combine_set_extension): Correct test for changing number
+   of hard registers when widening a reaching definition.
+
 2014-01-16  Bernd Schmidt  
 
PR middle-end/56791
diff --git a/gcc/ree.c b/gcc/ree.c
index 19d821c..96cddd2 100644
--- a/gcc/ree.c
+++ b/gcc/ree.c
@@ -300,7 +300,8 @@ combine_set_extension (ext_cand *cand, rtx curr_insn, rtx 
*orig_set)
   /* We're going to be widening the result of DEF_INSN, ensure that doing so
  doesn't change the number of hard registers needed for the result.  */
   if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
-  != HARD_REGNO_NREGS (REGNO (orig_src), GET_MODE (SET_DEST (*orig_set
+  != HARD_REGNO_NREGS (REGNO (SET_DEST (*orig_set)),
+  GET_MODE (SET_DEST (*orig_set
return false;
 
   /* Merge constants by directly moving the constant into the register under

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Jeff Law]

On 01/16/14 04:52, Eric Botcazou wrote:

Yes, like in the attached patch?  OK for the trunk?


Unfortunately this broke again bootstrap with RTL checking enabled on x86-64:

/home/eric/svn/gcc/libgcc/libgcc2.c: In function '__negdi2':
/home/eric/svn/gcc/libgcc/libgcc2.c:71:1: internal compiler error: RTL check:
expected code 'reg', have 'ne' in rhs_regno, at rtl.h:1125
  }

Patch testing in progress.  Should be wrapped up before I finish today.

jeff

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Jeff Law]

On 01/16/14 11:23, Dominique Dhumieres wrote:

The test gcc.c-torture/execute/pr59747.c fails on darwin13 at execution time 
(Abort)
for all tested optimizations, -m32/-m64, and gcc versions before or after the 
commit.

Should I
(1) reopen pr59749,
(2) open a new pr,
(3) let you handle the problem without further fuss?

For (1) or (2) do you need a preprocessed file?

I'm certainly not seeing 59747 failing anymore:

PASS: gcc.c-torture/execute/pr59747.c compilation,  -O0
PASS: gcc.c-torture/execute/pr59747.c execution,  -O0
PASS: gcc.c-torture/execute/pr59747.c compilation,  -O1
PASS: gcc.c-torture/execute/pr59747.c execution,  -O1
PASS: gcc.c-torture/execute/pr59747.c compilation,  -O2
PASS: gcc.c-torture/execute/pr59747.c execution,  -O2
PASS: gcc.c-torture/execute/pr59747.c compilation,  -O3 
-fomit-frame-pointer

PASS: gcc.c-torture/execute/pr59747.c execution,  -O3 -fomit-frame-pointer
PASS: gcc.c-torture/execute/pr59747.c compilation,  -O3 -g
PASS: gcc.c-torture/execute/pr59747.c execution,  -O3 -g
PASS: gcc.c-torture/execute/pr59747.c compilation,  -Os
PASS: gcc.c-torture/execute/pr59747.c execution,  -Os
PASS: gcc.c-torture/execute/pr59747.c compilation,  -Og -g
PASS: gcc.c-torture/execute/pr59747.c execution,  -Og -g
PASS: gcc.c-torture/execute/pr59747.c compilation,  -O2 -flto 
-fno-use-linker-plugin -flto-partition=none
PASS: gcc.c-torture/execute/pr59747.c execution,  -O2 -flto 
-fno-use-linker-plugin -flto-partition=none
PASS: gcc.c-torture/execute/pr59747.c compilation,  -O2 -flto 
-fuse-linker-plugin -fno-fat-lto-objects
PASS: gcc.c-torture/execute/pr59747.c execution,  -O2 -flto 
-fuse-linker-plugin -fno-fat-lto-objects



I don't have a darwin box so I'd probably need the .i & .s files so that 
I could poke at it a bit with a cross compiler.


I think reopening 59749 would be appropriate.

jeff

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Dominique Dhumieres]

The test gcc.c-torture/execute/pr59747.c fails on darwin13 at execution time 
(Abort)
for all tested optimizations, -m32/-m64, and gcc versions before or after the 
commit.

Should I
(1) reopen pr59749,
(2) open a new pr,
(3) let you handle the problem without further fuss?

For (1) or (2) do you need a preprocessed file?

TIA

Dominique

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Jeff Law]

On 01/16/14 04:52, Eric Botcazou wrote:

Yes, like in the attached patch?  OK for the trunk?


Unfortunately this broke again bootstrap with RTL checking enabled on x86-64:

/home/eric/svn/gcc/libgcc/libgcc2.c: In function '__negdi2':
/home/eric/svn/gcc/libgcc/libgcc2.c:71:1: internal compiler error: RTL check:
expected code 'reg', have 'ne' in rhs_regno, at rtl.h:1125

I'm on it.  Looks like a stupidity leak on my part.

jeff

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Eric Botcazou]

> Yes, like in the attached patch?  OK for the trunk?

Unfortunately this broke again bootstrap with RTL checking enabled on x86-64:

/home/eric/svn/gcc/libgcc/libgcc2.c: In function '__negdi2':
/home/eric/svn/gcc/libgcc/libgcc2.c:71:1: internal compiler error: RTL check: 
expected code 'reg', have 'ne' in rhs_regno, at rtl.h:1125
 }
 ^
0x9cb813 rtl_check_failed_code1(rtx_def const*, rtx_code, char const*, int, 
char const*)
/home/eric/svn/gcc/gcc/rtl.c:773
0x146a6ab rhs_regno
/home/eric/svn/gcc/gcc/rtl.h:1125
0x146a6ab combine_set_extension
/home/eric/svn/gcc/gcc/ree.c:303
0x146a6ab merge_def_and_ext
/home/eric/svn/gcc/gcc/ree.c:658
0x146bfbd combine_reaching_defs
/home/eric/svn/gcc/gcc/ree.c:786
0x146d149 find_and_remove_re
/home/eric/svn/gcc/gcc/ree.c:993
0x146d149 rest_of_handle_ree
/home/eric/svn/gcc/gcc/ree.c:1064
0x146d149 execute
/home/eric/svn/gcc/gcc/ree.c:1103
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See  for instructions.
make[5]: *** [_negdi2.o] Error 1

-- 
Eric Botcazou

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Andreas Schwab]

* gcc.c-torture/execute/pr59747.c (fn1): Return a value.

Index: gcc.c-torture/execute/pr59747.c
===
--- gcc.c-torture/execute/pr59747.c (revision 206658)
+++ gcc.c-torture/execute/pr59747.c (working copy)
@@ -1,13 +1,13 @@
 extern void abort (void);
 extern void exit (int);
 
-int a[6], b, c = 1, d;
+int a[6], c = 1, d;
 short e;
 
 int __attribute__ ((noinline))
 fn1 (int p)
 {
-  b = a[p];
+  return a[p];
 }
 
 int

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Jakub Jelinek]

On Wed, Jan 15, 2014 at 10:56:35AM -0700, Jeff Law wrote:
> On 01/13/14 00:34, Jakub Jelinek wrote:
> >On Sun, Jan 12, 2014 at 11:21:59PM -0700, Jeff Law wrote:
> >>--- a/gcc/ree.c
> >>+++ b/gcc/ree.c
> >>@@ -297,6 +297,13 @@ combine_set_extension (ext_cand *cand, rtx curr_insn, 
> >>rtx *orig_set)
> >>else
> >>  new_reg = gen_rtx_REG (cand->mode, REGNO (SET_DEST (*orig_set)));
> >>
> >>+  /* We're going to be widening the result of DEF_INSN, ensure that doing 
> >>so
> >>+ doesn't change the number of hard registers needed for the result.  */
> >>+  if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
> >>+  != HARD_REGNO_NREGS (REGNO (SET_SRC (*orig_set)),
> >
> >Note you can use orig_src instead of SET_SRC (*orig_set) here.
> Yea.  A little manual CSE never hurts.  Fixed.

The patch is ok with that fix then.  Thanks.

Jakub

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Jeff Law]

On 01/13/14 00:34, Jakub Jelinek wrote:

On Sun, Jan 12, 2014 at 11:21:59PM -0700, Jeff Law wrote:

--- a/gcc/ree.c
+++ b/gcc/ree.c
@@ -297,6 +297,13 @@ combine_set_extension (ext_cand *cand, rtx curr_insn, rtx 
*orig_set)
else
  new_reg = gen_rtx_REG (cand->mode, REGNO (SET_DEST (*orig_set)));

+  /* We're going to be widening the result of DEF_INSN, ensure that doing so
+ doesn't change the number of hard registers needed for the result.  */
+  if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
+  != HARD_REGNO_NREGS (REGNO (SET_SRC (*orig_set)),


Note you can use orig_src instead of SET_SRC (*orig_set) here.

Yea.  A little manual CSE never hurts.  Fixed.





+  GET_MODE (SET_DEST (*orig_set
+   return false;
+
/* Merge constants by directly moving the constant into the register under
   some conditions.  Recall that RTL constants are sign-extended.  */
if (GET_CODE (orig_src) == CONST_INT


Are you sure the above is needed even for the
REGNO (new_reg) == REGNO (SET_DEST (*orig_set))
&& REGNO (new_reg) == REGNO (orig_src) case?
It wasn't clear when I was reviewing the code, so I took the 
conservative approach of always rejecting when the # hard regs changes 
as a result of widening.




(set (reg:SI 3) (something:SI))
(set (reg:SI 2) (expression:SI))// def_insn
(use (reg:SI 3))
(set (reg:DI 3) (sign_extend:DI (reg:SI 2)))

So, perhaps if we wanted to handle the HARD_REGNO_NREGS != HARD_REGNO_NREGS
case when all 3 REGNOs are the same, we'd need to limit it to the case where
cand->insn and curr_insn are in the same bb, DF_INSN_LUID of curr_insn
is smaller than DF_INSN_LUID of cand->insn and the extra hard regs aren't
used between the two.  Perhaps not worth it?

I doubt it's worth it.  A size change here is pretty unusual.



BTW, I'm surprised to hear that it triggers in the testsuite already (for
the 3 REGNOs the same case, or different?), is that on x86_64 or i?86?
Do you have an example?  I'm surprised that we'd have post reload a pattern
that extends into multiple hard registers.
There were only a couple on x86_64, both related to handling vector 
regs.  They would actually fail later, but they tripped the changing 
size check.


Here's one example:

(insn 16 15 17 2 (set (reg:V4SI 21 xmm0 [99])
(vec_select:V4SI (reg:V8SI 22 xmm1 [orig:85 vect__3.443 ] [85])
(parallel [
(const_int 4 [0x4])
(const_int 5 [0x5])
(const_int 6 [0x6])
(const_int 7 [0x7])
]))) j.c:26 1926 {vec_extract_hi_v8si}
 (nil))
(insn 17 16 18 2 (set (reg:V4DI 21 xmm0 [orig:98 vect__4.444 ] [98])
(sign_extend:V4DI (reg:V4SI 21 xmm0 [99]))) j.c:26 2583 
{avx2_sign_extendv4siv4di2}

 (nil))



Changing (reg:V4SI xmm0) to (reg:V4DI xmm0) changes the number of hard 
regs and thus tripped the check.


The transformation would actually fail later when it tried to actually 
combine the extension and vec_select into this insn:


(insn 16 15 17 2 (set (reg:V4DI 21 xmm0)
(sign_extend:V4DI (vec_select:V4SI (reg:V8SI 22 xmm1 [orig:85 
vect__3.443 ] [85])

(parallel [
(const_int 4 [0x4])
(const_int 5 [0x5])
(const_int 6 [0x6])
(const_int 7 [0x7])
] j.c:26 -1

I guess I could run a -m32 multilib test and see if I can get it to 
trigger in on something that will create a valid insn.



Jeff

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Jakub Jelinek]

On Sun, Jan 12, 2014 at 11:21:59PM -0700, Jeff Law wrote:
> --- a/gcc/ree.c
> +++ b/gcc/ree.c
> @@ -297,6 +297,13 @@ combine_set_extension (ext_cand *cand, rtx curr_insn, 
> rtx *orig_set)
>else
>  new_reg = gen_rtx_REG (cand->mode, REGNO (SET_DEST (*orig_set)));
>  
> +  /* We're going to be widening the result of DEF_INSN, ensure that doing so
> + doesn't change the number of hard registers needed for the result.  */
> +  if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
> +  != HARD_REGNO_NREGS (REGNO (SET_SRC (*orig_set)),

Note you can use orig_src instead of SET_SRC (*orig_set) here.

> +GET_MODE (SET_DEST (*orig_set
> + return false;
> +
>/* Merge constants by directly moving the constant into the register under
>   some conditions.  Recall that RTL constants are sign-extended.  */
>if (GET_CODE (orig_src) == CONST_INT

Are you sure the above is needed even for the
REGNO (new_reg) == REGNO (SET_DEST (*orig_set))
&& REGNO (new_reg) == REGNO (orig_src) case?
I mean in that case no copy insn is going to be scheduled right now, nor has
been previously scheduled, so we are back to what the code did before
r206418.  I can imagine it can be a problem, but doesn't have to be.

(set (reg:SI 3) (something:SI))
(set (reg:SI 2) (expression:SI))// def_insn
(use (reg:SI 3))
(set (reg:DI 3) (sign_extend:DI (reg:SI 2)))

So, perhaps if we wanted to handle the HARD_REGNO_NREGS != HARD_REGNO_NREGS
case when all 3 REGNOs are the same, we'd need to limit it to the case where
cand->insn and curr_insn are in the same bb, DF_INSN_LUID of curr_insn
is smaller than DF_INSN_LUID of cand->insn and the extra hard regs aren't
used between the two.  Perhaps not worth it?

BTW, I'm surprised to hear that it triggers in the testsuite already (for
the 3 REGNOs the same case, or different?), is that on x86_64 or i?86?
Do you have an example?  I'm surprised that we'd have post reload a pattern
that extends into multiple hard registers.

Jakub

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Jeff Law]

On 01/10/14 14:52, Jakub Jelinek wrote:

There is one thing I still worry about, if some target has
an insn to say sign extend or zero extend a short memory load
into HARD_REGNO_NREGS () > 1 register, but the other involved
register has the only one (or fewer) hard registers available to it.
Consider registers SImode hard registers 0, 1, 2, 3:
   (set (reg:SI 3) (something:SI))
   (set (reg:HI 0) (expression:HI))
   (set (reg:SI 2) (sign_extend:SI (reg:HI 0)))
   (set (reg:DI 0) (sign_extend:DI (reg:HI 0)))
   (use (reg:SI 3))
we transform this into:
   (set (reg:SI 3) (something:SI))
   (set (reg:SI 2) (sign_extend:SI (expression:HI)))
   (set (reg:SI 0) (reg:HI 2))
   (set (reg:DI 0) (sign_extend:DI (reg:HI 0)))
   (use (reg:SI 3))
first (well, the middle is then pending in copy list), and next:
   (set (reg:SI 3) (something))
   (set (reg:DI 2) (sign_extend:DI (expression:HI)))
   (set (reg:DI 0) (reg:DI 2))
   (use (reg:SI 3))
but that looks wrong, because the second instruction would now clobber
(reg:SI 3).  Dunno if we have such an target and thus if it is possible
to construct a testcase.
No need to construct a testcase, there's a few that trip the condition 
in the existing testsuite :-)


Basically I just put a check in combine_set_extension to detect when 
widening of the result of the reaching def requires more hard registers 
than it previously needed and ran the testsuite.





So, I'd say the handling of the second extend should notice that
it is actually extending load into a different register and bail out
if it would need more hard registers than it needed previously, or
something similar.

Yes, like in the attached patch?  OK for the trunk?


commit 1313449102ac8d62e36818d8660ef2e897bd59e3
Author: Jeff Law 
Date:   Fri Jan 10 14:31:15 2014 -0700

PR tree-optimization/59747
* ree.c (find_and_remove_re): Properly handle case where a second
eliminated extension requires widening a copy created for elimination
of a prior extension.
(combine_set_extension): Ensure that the number of hard regs needed
for a destination register does not change when we widen it.

PR tree-optimization/59747
* gcc.c-torture/execute/pr59747.c: New test.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index c554609..a82e23c 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -5,6 +5,13 @@
occurs before the extension when optimizing extensions with
different source and destination hard registers.
 
+   PR tree-optimization/59747
+   * ree.c (find_and_remove_re): Properly handle case where a second
+   eliminated extension requires widening a copy created for elimination
+   of a prior extension.
+   (combine_set_extension): Ensure that the number of hard regs needed
+   for a destination register does not change when we widen it.
+
 2014-01-10  Jan Hubicka  
 
PR ipa/58585
diff --git a/gcc/ree.c b/gcc/ree.c
index 63cc8cc..3ee97cd 100644
--- a/gcc/ree.c
+++ b/gcc/ree.c
@@ -297,6 +297,13 @@ combine_set_extension (ext_cand *cand, rtx curr_insn, rtx 
*orig_set)
   else
 new_reg = gen_rtx_REG (cand->mode, REGNO (SET_DEST (*orig_set)));
 
+  /* We're going to be widening the result of DEF_INSN, ensure that doing so
+ doesn't change the number of hard registers needed for the result.  */
+  if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
+  != HARD_REGNO_NREGS (REGNO (SET_SRC (*orig_set)),
+  GET_MODE (SET_DEST (*orig_set
+   return false;
+
   /* Merge constants by directly moving the constant into the register under
  some conditions.  Recall that RTL constants are sign-extended.  */
   if (GET_CODE (orig_src) == CONST_INT
@@ -1017,11 +1024,20 @@ find_and_remove_re (void)
   for (unsigned int i = 0; i < reinsn_copy_list.length (); i += 2)
 {
   rtx curr_insn = reinsn_copy_list[i];
+  rtx def_insn = reinsn_copy_list[i + 1];
+
+  /* Use the mode of the destination of the defining insn
+for the mode of the copy.  This is necessary if the
+defining insn was used to eliminate a second extension
+that was wider than the first.  */
+  rtx sub_rtx = *get_sub_rtx (def_insn);
   rtx pat = PATTERN (curr_insn);
-  rtx new_reg = gen_rtx_REG (GET_MODE (SET_DEST (pat)),
+  rtx new_dst = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)),
 REGNO (XEXP (SET_SRC (pat), 0)));
-  rtx set = gen_rtx_SET (VOIDmode, new_reg, SET_DEST (pat));
-  emit_insn_after (set, reinsn_copy_list[i + 1]);
+  rtx new_src = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)),
+REGNO (SET_DEST (pat)));
+  rtx set = gen_rtx_SET (VOIDmode, new_dst, new_src);
+  emit_insn_after (set, def_insn);
 }
 
   /* Delete all useless extensions here in one sweep.  */
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index f40d56e..a603952 100644
--- a/gcc/testsuite/ChangeLog
+++ 

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Jeff Law]

On 01/10/14 14:52, Jakub Jelinek wrote:


There is one thing I still worry about, if some target has
an insn to say sign extend or zero extend a short memory load
into HARD_REGNO_NREGS () > 1 register, but the other involved
register has the only one (or fewer) hard registers available to it.
Consider registers SImode hard registers 0, 1, 2, 3:
   (set (reg:SI 3) (something:SI))
   (set (reg:HI 0) (expression:HI))
   (set (reg:SI 2) (sign_extend:SI (reg:HI 0)))
   (set (reg:DI 0) (sign_extend:DI (reg:HI 0)))
   (use (reg:SI 3))
we transform this into:
   (set (reg:SI 3) (something:SI))
   (set (reg:SI 2) (sign_extend:SI (expression:HI)))
   (set (reg:SI 0) (reg:HI 2))
   (set (reg:DI 0) (sign_extend:DI (reg:HI 0)))
   (use (reg:SI 3))
first (well, the middle is then pending in copy list), and next:
   (set (reg:SI 3) (something))
   (set (reg:DI 2) (sign_extend:DI (expression:HI)))
   (set (reg:DI 0) (reg:DI 2))
   (use (reg:SI 3))
but that looks wrong, because the second instruction would now clobber
(reg:SI 3).  Dunno if we have such an target and thus if it is possible
to construct a testcase.
Seems like any port where we can do operations on 3 different modes and 
the largest mode requires > 1 hard reg ought to do.  I don't see 
anything inherent in this setup that wouldn't apply to x86 for example.




So, I'd say the handling of the second extend should notice that
it is actually extending load into a different register and bail out
if it would need more hard registers than it needed previously, or
something similar.
That ought to be a trivial check to add.   Let me add it and see if I 
can find something to trip it.





The printf prototype isn't needed, and the noreturn attributes
aren't needed either, as they are builtins, the compiler will
add those automatically.
The printf prototype came from the original and I forgot to remove it 
when I zapped the printf statement.  As for abort/exit, they're not 
strictly needed, but they shut up the compiler when running things by 
hand during testing/development. They're easy enough to zap.


Thanks,
Jeff

Re: [RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Jakub Jelinek]

On Fri, Jan 10, 2014 at 02:36:47PM -0700, Jeff Law wrote:
> As mentioned in the PR.  We have a memory load and two extensions.
> 
> The first extension requires a copy because its source and
> destination are not the same hard register.
> 
> The second extension does not require a copy and has a wider mode
> than the first extension.
> 
> In this case we have to make sure to widen the copy we emit to
> eliminate the first extension.  Otherwise upper bits aren't going to
> have their expected values.  Thankfully the copy isn't emitted until
> the end of ree and we have the modified memory reference to peek at
> to get the proper mode.
> 
> Bootstrapped and regression tested on x86_64-unknown-linux-gnu.  OK
> for the trunk?

> --- a/gcc/ree.c
> +++ b/gcc/ree.c
> @@ -1003,11 +1003,20 @@ find_and_remove_re (void)
>for (unsigned int i = 0; i < reinsn_copy_list.length (); i += 2)
>  {
>rtx curr_insn = reinsn_copy_list[i];
> +  rtx def_insn = reinsn_copy_list[i + 1];
> +
> +  /* Use the mode of the destination of the defining insn
> +  for the mode of the copy.  This is necessary if the
> +  defining insn was used to eliminate a second extension
> +  that was wider than the first.  */
> +  rtx sub_rtx = *get_sub_rtx (def_insn);
>rtx pat = PATTERN (curr_insn);
> -  rtx new_reg = gen_rtx_REG (GET_MODE (SET_DEST (pat)),
> +  rtx new_dst = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)),
>REGNO (XEXP (SET_SRC (pat), 0)));
> -  rtx set = gen_rtx_SET (VOIDmode, new_reg, SET_DEST (pat));
> -  emit_insn_after (set, reinsn_copy_list[i + 1]);
> +  rtx new_src = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)),
> +  REGNO (SET_DEST (pat)));
> +  rtx set = gen_rtx_SET (VOIDmode, new_dst, new_src);
> +  emit_insn_after (set, def_insn);
>  }

There is one thing I still worry about, if some target has
an insn to say sign extend or zero extend a short memory load
into HARD_REGNO_NREGS () > 1 register, but the other involved
register has the only one (or fewer) hard registers available to it.
Consider registers SImode hard registers 0, 1, 2, 3:
  (set (reg:SI 3) (something:SI))
  (set (reg:HI 0) (expression:HI))
  (set (reg:SI 2) (sign_extend:SI (reg:HI 0)))
  (set (reg:DI 0) (sign_extend:DI (reg:HI 0)))
  (use (reg:SI 3))
we transform this into:
  (set (reg:SI 3) (something:SI))
  (set (reg:SI 2) (sign_extend:SI (expression:HI)))
  (set (reg:SI 0) (reg:HI 2))
  (set (reg:DI 0) (sign_extend:DI (reg:HI 0)))
  (use (reg:SI 3))
first (well, the middle is then pending in copy list), and next:
  (set (reg:SI 3) (something))
  (set (reg:DI 2) (sign_extend:DI (expression:HI)))
  (set (reg:DI 0) (reg:DI 2))
  (use (reg:SI 3))
but that looks wrong, because the second instruction would now clobber
(reg:SI 3).  Dunno if we have such an target and thus if it is possible
to construct a testcase.

So, I'd say the handling of the second extend should notice that
it is actually extending load into a different register and bail out
if it would need more hard registers than it needed previously, or
something similar.

> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr59747.c
> @@ -0,0 +1,28 @@
> +extern void abort (void) __attribute__ ((__noreturn__));
> +extern void exit (int) __attribute__ ((__noreturn__));
> +extern int printf (const char *, ...);

The printf prototype isn't needed, and the noreturn attributes
aren't needed either, as they are builtins, the compiler will
add those automatically.

Jakub

[RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

[Jeff Law]

As mentioned in the PR.  We have a memory load and two extensions.

The first extension requires a copy because its source and destination 
are not the same hard register.


The second extension does not require a copy and has a wider mode than 
the first extension.


In this case we have to make sure to widen the copy we emit to eliminate 
the first extension.  Otherwise upper bits aren't going to have their 
expected values.  Thankfully the copy isn't emitted until the end of ree 
and we have the modified memory reference to peek at to get the proper mode.


Bootstrapped and regression tested on x86_64-unknown-linux-gnu.  OK for 
the trunk?


commit 7a83f984205b101f61dbfcabef59e8f459950f88
Author: Jeff Law 
Date:   Fri Jan 10 14:31:15 2014 -0700

PR tree-optimization/59747
* ree.c (find_and_remove_re): Properly handle case where a second
eliminated extension requires widening a copy created for elimination
of a prior extension.

PR tree-optimization/59747
* gcc.c-torture/execute/pr59747.c: New test.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index a125517..4294831 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,10 @@
+2014-01-10  Jeff Law  
+
+   PR tree-optimization/59747
+   * ree.c (find_and_remove_re): Properly handle case where a second
+   eliminated extension requires widening a copy created for elimination
+   of a prior extension.
+ 
 2014-01-09  Rong Xu  
 
* libgcc/libgcov-driver.c (this_prg): make it local to save
diff --git a/gcc/ree.c b/gcc/ree.c
index 1c4f3ad..0a34131 100644
--- a/gcc/ree.c
+++ b/gcc/ree.c
@@ -1003,11 +1003,20 @@ find_and_remove_re (void)
   for (unsigned int i = 0; i < reinsn_copy_list.length (); i += 2)
 {
   rtx curr_insn = reinsn_copy_list[i];
+  rtx def_insn = reinsn_copy_list[i + 1];
+
+  /* Use the mode of the destination of the defining insn
+for the mode of the copy.  This is necessary if the
+defining insn was used to eliminate a second extension
+that was wider than the first.  */
+  rtx sub_rtx = *get_sub_rtx (def_insn);
   rtx pat = PATTERN (curr_insn);
-  rtx new_reg = gen_rtx_REG (GET_MODE (SET_DEST (pat)),
+  rtx new_dst = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)),
 REGNO (XEXP (SET_SRC (pat), 0)));
-  rtx set = gen_rtx_SET (VOIDmode, new_reg, SET_DEST (pat));
-  emit_insn_after (set, reinsn_copy_list[i + 1]);
+  rtx new_src = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)),
+REGNO (SET_DEST (pat)));
+  rtx set = gen_rtx_SET (VOIDmode, new_dst, new_src);
+  emit_insn_after (set, def_insn);
 }
 
   /* Delete all useless extensions here in one sweep.  */
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index acb1637..1023133 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2014-01-10  Jeff Law  
+
+   PR tree-optimization/59747
+   * gcc.c-torture/execute/pr59747.c: New test.
+
 2014-10-09  Jakub Jelinek  
 
PR sanitizer/59136
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr59747.c 
b/gcc/testsuite/gcc.c-torture/execute/pr59747.c
new file mode 100644
index 000..edb1685
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr59747.c
@@ -0,0 +1,28 @@
+extern void abort (void) __attribute__ ((__noreturn__));
+extern void exit (int) __attribute__ ((__noreturn__));
+extern int printf (const char *, ...);
+
+int a[6], b, c = 1, d;
+short e;
+
+int __attribute__ ((noinline))
+fn1 (int p)
+{
+  b = a[p];
+}
+
+int
+main ()
+{
+  if (sizeof (long long) != 8)
+exit (0);
+
+  a[0] = 1;
+  if (c)
+e--;
+  d = e;
+  long long f = e;
+  if (fn1 ((f >> 56) & 1) != 0)
+abort ();
+  exit (0);
+}