RFT: Fix PR middle/end-40154

RFT: Fix PR middle/end-40154

[joern.rennecke]

[-- Attachment #1.1: Type: text/plain, Size: 208 bytes --]

This fixes the problem on the Epiphany, but I haven't tested if it also fixes it on
the SH - AAUI you'd have to back out a workaround first to properly test it.

Bootstrapped powerpc64-unknown-linux-gnu.

[-- Attachment #2: pr40154-fix --]
[-- Type: application/octet-stream, Size: 1292 bytes --]

2011-11-07  Joern Rennecke  <joern.rennecke@embecosm.com>

	PR middle-end/40154
	* emit-rtl.c (set_unique_reg_note): Don't add notes that disagree
	with the SET_DEST of INSN.

Index: emit-rtl.c
===================================================================
--- emit-rtl.c	(revision 181122)
+++ emit-rtl.c	(working copy)
@@ -4951,8 +4951,27 @@
 rtx
 set_unique_reg_note (rtx insn, enum reg_note kind, rtx datum)
 {
-  rtx note = find_reg_note (insn, kind, NULL_RTX);
+  rtx set, note;
+  enum machine_mode mode;
 
+  /* Sometimes the value is calculated with some SUBREG tricks, so the
+     SET_DEST of INSN ends up with a different mode then DATUM.  */
+  set = single_set (insn);
+  gcc_assert (set);
+  mode = GET_MODE (SET_DEST (set));
+  /* If DATUM is too narrow, we can't make it fit.  */
+  if ((GET_MODE (datum) != VOIDmode || GET_MODE_CLASS (mode) != MODE_INT)
+      && GET_MODE_SIZE (mode) > GET_MODE_SIZE (GET_MODE (datum)))
+    return NULL_RTX;
+  if (GET_MODE (datum) != VOIDmode && GET_MODE (datum) != mode)
+    {
+      /* Adjust DATUM to the SET_DEST.  */
+      datum = simplify_gen_subreg (mode, datum, GET_MODE (datum), 0);
+      if (!datum)
+	return NULL_RTX;
+    }
+  note = find_reg_note (insn, kind, NULL_RTX);
+
   switch (kind)
     {
     case REG_EQUAL:

RFT: Fix PR middle/end-40154

[Kaz Kojima]

"joern.rennecke@embecosm.com" <joern.rennecke@embecosm.com> wrote:
> This fixes the problem on the Epiphany, but I haven't tested if it also fixes it on
> the SH - AAUI you'd have to back out a workaround first to properly test it.
> 
> Bootstrapped powerpc64-unknown-linux-gnu.

With the patch + reverting 148018 workaround, I've got a failure
when building libgcc on SH.

/exp/ldroot/dodes/xsh-gcc/./gcc/xgcc -B/exp/ldroot/dodes/xsh-gcc/./gcc/ -B/usr/local/sh4-unknown-linux-gnu/bin/ -B/usr/local/sh4-unknown-linux-gnu/lib/ -isystem /usr/local/sh4-unknown-linux-gnu/include -isystem /usr/local/sh4-unknown-linux-gnu/sys-include    -g -O2 -O2  -g -O2 -DIN_GCC -DCROSS_DIRECTORY_STRUCTURE  -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition  -isystem ./include   -fpic -mieee -mieee -DNO_FPSCR_VALUES -g -DIN_LIBGCC2 -fbuilding-libgcc -fno-stack-protector   -fpic -mieee -mieee -DNO_FPSCR_VALUES -I. -I. -I../.././gcc -I../../../LOCAL/trunk/libgcc -I../../../LOCAL/trunk/libgcc/. -I../../../LOCAL/trunk/libgcc/../gcc -I../../../LOCAL/trunk/libgcc/../include  -DHAVE_CC_TLS  -o _muldc3.o -MT _muldc3.o -MD -MP -MF _muldc3.dep -DL_muldc3 -c ../../../LOCAL/trunk/libgcc/libgcc2.c -fvisibility=hidden -DHIDE_EXPORTS
../../../LOCAL/trunk/libgcc/libgcc2.c: In function '__muldc3':
../../../LOCAL/trunk/libgcc/libgcc2.c:1929:1: internal compiler error: in set_unique_reg_note, at emit-rtl.c:4960

#0  fancy_abort (file=0x8929248 "../../LOCAL/trunk/gcc/emit-rtl.c", line=4960, 
    function=0x8928f44 "set_unique_reg_note")
    at ../../LOCAL/trunk/gcc/diagnostic.c:899
#1  0x0826dead in set_unique_reg_note (insn=0xb7fda120, kind=REG_EQUAL, 
    datum=0xb7fd5aa4) at ../../LOCAL/trunk/gcc/emit-rtl.c:4960
#2  0x08486dc6 in find_reloads (insn=0xb7fa59fc, replace=1, ind_levels=0, 
    live_known=1, reload_reg_p=0x8ad2660)
    at ../../LOCAL/trunk/gcc/reload.c:2866
#3  0x0849596f in reload_as_needed (live_known=1)
    at ../../LOCAL/trunk/gcc/reload1.c:4631
#4  0x08498422 in reload (first=0xb7f436a0, global=1)
    at ../../LOCAL/trunk/gcc/reload1.c:1058

(gdb) fr 1
#1  0x0826dead in set_unique_reg_note (insn=0xb7fda120, kind=REG_EQUAL, 
    datum=0xb7fd5aa4) at ../../LOCAL/trunk/gcc/emit-rtl.c:4960
4960	  gcc_assert (set);
(gdb) call debug_rtx(insn)
(insn 601 576 67 10 (use (reg/v:DF 219 [ a ])) ../../../LOCAL/trunk/libgcc/libgcc2.c:1892 -1
     (nil))
(gdb) fr 2
#2  0x08486dc6 in find_reloads (insn=0xb7fa59fc, replace=1, ind_levels=0, 
    live_known=1, reload_reg_p=0x8ad2660)
    at ../../LOCAL/trunk/gcc/reload.c:2866
2866		    set_unique_reg_note (emit_insn_before (gen_rtx_USE (VOIDmode, reg),

It seems that find_reloads calls set_unique_reg_note for
a USE insn.

Regards,
	kaz

Re: RFT: Fix PR middle/end-40154

[Joern Rennecke]

[-- Attachment #1: Type: text/plain, Size: 373 bytes --]

Quoting Kaz Kojima <kkojima@rr.iij4u.or.jp>:

> It seems that find_reloads calls set_unique_reg_note for
> a USE insn.

That's true, and it is by design.
This use of set_unique_reg_note is a bit debatable - add_reg_note
should do just fine there.

OTOH keeping this as it is, and keeping set_unique_reg_note accepting USE
in this case, seems more conservative for stage3.


[-- Attachment #2: pr40154-fix-2 --]
[-- Type: text/plain, Size: 1528 bytes --]

2011-11-07  Joern Rennecke  <joern.rennecke@embecosm.com>

	* emit-rtl.c (set_unique_reg_note): Don't add notes that disagree
	with the SET_DEST of INSN.

Index: trunk/gcc/emit-rtl.c
===================================================================
--- trunk/gcc/emit-rtl.c	(revision 181122)
+++ trunk/gcc/emit-rtl.c	(working copy)
@@ -4951,8 +4951,32 @@
 rtx
 set_unique_reg_note (rtx insn, enum reg_note kind, rtx datum)
 {
-  rtx note = find_reg_note (insn, kind, NULL_RTX);
+  rtx set, note;
+  enum machine_mode mode;
 
+  /* Sometimes the value is calculated with some SUBREG tricks, so the
+     SET_DEST of INSN ends up with a different mode then DATUM.  */
+  set = single_set (insn);
+  if (set)
+    {
+      mode = GET_MODE (SET_DEST (set));
+      /* If DATUM is too narrow, we can't make it fit.  */
+      if ((GET_MODE (datum) != VOIDmode || GET_MODE_CLASS (mode) != MODE_INT)
+	  && GET_MODE_SIZE (mode) > GET_MODE_SIZE (GET_MODE (datum)))
+	return NULL_RTX;
+      if (GET_MODE (datum) != VOIDmode && GET_MODE (datum) != mode)
+	{
+	  /* Adjust DATUM to the SET_DEST.  */
+	  datum = simplify_gen_subreg (mode, datum, GET_MODE (datum), 0);
+	  if (!datum)
+	    return NULL_RTX;
+	}
+    }
+  else /* Reload uses USEs with REG_EQUAL notes attached to keep track of
+	  reload inhertiance opportunities.  */
+    gcc_assert (PATTERN (insn) == USE && reload_in_progress);
+  note = find_reg_note (insn, kind, NULL_RTX);
+
   switch (kind)
     {
     case REG_EQUAL:

Re: RFT: Fix PR middle/end-40154

[Kaz Kojima]

Joern Rennecke <joern.rennecke@embecosm.com> wrote:
> That's true, and it is by design.
> This use of set_unique_reg_note is a bit debatable - add_reg_note
> should do just fine there.
> 
> OTOH keeping this as it is, and keeping set_unique_reg_note accepting USE
> in this case, seems more conservative for stage3.

A tiny change was needed

--- trunk/gcc/emit-rtl.c.orig	2011-11-08 22:46:20.000000000 +0900
+++ trunk/gcc/emit-rtl.c	2011-11-08 22:53:16.000000000 +0900
@@ -4974,7 +4974,7 @@ set_unique_reg_note (rtx insn, enum reg_
     }
   else /* Reload uses USEs with REG_EQUAL notes attached to keep track of
 	  reload inhertiance opportunities.  */
-    gcc_assert (PATTERN (insn) == USE && reload_in_progress);
+    gcc_assert (GET_CODE (PATTERN (insn)) == USE && reload_in_progress);
   note = find_reg_note (insn, kind, NULL_RTX);
 
   switch (kind)

I'm regtesting the patch on SH, though currently many C++ tests fail
on SH with

undefined reference to `std::atomic_thread_fence(std::memory_order)'.

Regards,
	kaz

Re: RFT: Fix PR middle/end-40154

[Kaz Kojima]

> I'm regtesting the patch on SH, though currently many C++ tests fail
> on SH with
> 
> undefined reference to `std::atomic_thread_fence(std::memory_order)'.

There are no new failures with the patch + reverting
148018 workaround on sh4-unknown-linux-gnu.

Regards,
	kaz

Re: RFT: Fix PR middle/end-40154

[Eric Botcazou]

> This fixes the problem on the Epiphany, but I haven't tested if it also
> fixes it on the SH - AAUI you'd have to back out a workaround first to
> properly test it.

The fix belongs in the caller instead - it should make sure that what it is 
doing makes sense at all.

-- 
Eric Botcazou

Re: RFT: Fix PR middle/end-40154

[Joern Rennecke]

Quoting Eric Botcazou <ebotcazou@adacore.com>:

>> This fixes the problem on the Epiphany, but I haven't tested if it also
>> fixes it on the SH - AAUI you'd have to back out a workaround first to
>> properly test it.
>
> The fix belongs in the caller instead - it should make sure that what it is
> doing makes sense at all.

set_unique_reg note already makes a number of checks so that its  
multitude of callers doesn't have to.  E.g. it checks that there is  
indeed only one set,
not only one set of a live register.

As you can see in the PR, there are different pieces in the compiler that
end up in the same bogosity as they add notes with the wrong mode of data.
Having the check & fix in one place makes this better maintainable.

Re: RFT: Fix PR middle/end-40154

[Eric Botcazou]

> set_unique_reg note already makes a number of checks so that its
> multitude of callers doesn't have to.  E.g. it checks that there is
> indeed only one set, not only one set of a live register.

Indeed, but not on the DATUM.

> As you can see in the PR, there are different pieces in the compiler that
> end up in the same bogosity as they add notes with the wrong mode of data.

This is the bug: you shouldn't call set_unique_reg_note on a DATUM with a mode 
different from that of the destination.  This usually means that the logic in 
the caller is confused.  This is the same interface as emit_move_insn.

-- 
Eric Botcazou

Re: RFT: Fix PR middle/end-40154

[Joern Rennecke]

Quoting Eric Botcazou <ebotcazou@adacore.com>:

> This is the bug: you shouldn't call set_unique_reg_note on a DATUM   
> with a mode
> different from that of the destination.  This usually means that the logic in
> the caller is confused.  This is the same interface as emit_move_insn.

No, it isn't.  Expanders call other expanders to do fancy stuff.  When
everything is done, they tag a REG_EQUAL note on the last insn.
One of the purposes of set_unique_reg_note is lubricate this process:
the layered expanders can add multiple REG_EQUAL notes.  We only want
the most high-level note, so we discard the previous one from the
next lower level.  Well, we realize now that we only want the most
high-level note that makes sense, actually.  We either have to make it
make sense, or abandon adding it.

If every expander has to analyze the instructions that have been issued
to figure out if the new note needs to be modified, or cannot be applied
at all, you end up with umpteen duplications of the checks I added to
set_unique_reg_note prepended to its call sites.

Re: RFT: Fix PR middle/end-40154

[Eric Botcazou]

> No, it isn't.  Expanders call other expanders to do fancy stuff.  When
> everything is done, they tag a REG_EQUAL note on the last insn.
> One of the purposes of set_unique_reg_note is lubricate this process:
> the layered expanders can add multiple REG_EQUAL notes.  We only want
> the most high-level note, so we discard the previous one from the
> next lower level.  Well, we realize now that we only want the most
> high-level note that makes sense, actually.  We either have to make it
> make sense, or abandon adding it.

This would have been a valid design 20 years ago when notes were implemented.
But, for the past couple of decades, set_unique_reg_note hasn't touched the 
datum and all bugs over the years in this area have been fixed in the caller.
So this one should be fixed the same way, I see no reason to special-case it.

> If every expander has to analyze the instructions that have been issued
> to figure out if the new note needs to be modified, or cannot be applied
> at all, you end up with umpteen duplications of the checks I added to
> set_unique_reg_note prepended to its call sites.

Yes, expanders must know what they're doing, but this isn't new.

-- 
Eric Botcazou

Re: RFT: Fix PR middle/end-40154

[Joern Rennecke]

Quoting Eric Botcazou <ebotcazou@adacore.com>:

>> set_unique_reg note already makes a number of checks so that its
>> multitude of callers doesn't have to.  E.g. it checks that there is
>> indeed only one set, not only one set of a live register.
>
> Indeed, but not on the DATUM.

P.S.: The DATUM is in the right mode for the computation that is being done,
from a high-level perspective.  The problem is that we don't have an insn
that uses that mode to tag it on to, because the actual implementation of
the computation uses a different mode.
That is really not that much different from ending up with trying to put the
note on an instruction that not only sets the result, but also some  
other value.