Bug 37053 - [4.4 regression] ICE in reload_cse_simplify_operands, at postreload.c:395
[4.4 regression] ICE in reload_cse_simplify_operands, at postreload.c:395
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: middle-end
4.3.1
: P4 normal
: 4.5.0
Assigned To: Not yet assigned to anyone
http://gcc.gnu.org/ml/gcc-patches/200...
: ice-on-valid-code
: 40414 (view as bug list)
Depends on:
Blocks: 28690 40414
  Show dependency treegraph
 
Reported: 2008-08-08 01:01 UTC by Stephen R. Marenka
Modified: 2012-03-13 13:04 UTC (History)
12 users (show)

See Also:
Host:
Target: m68k-linux-gnu
Build:
Known to work: 4.5.0
Known to fail:
Last reconfirmed: 2008-09-11 15:19:57


Attachments
Preprocessed file (14.86 KB, text/plain)
2008-08-08 01:02 UTC, Stephen R. Marenka
Details
reduced source from r-base (26.17 KB, text/plain)
2008-08-08 01:02 UTC, Stephen R. Marenka
Details
Proposed patch (8.18 KB, patch)
2009-06-24 16:02 UTC, Maxim Kuvyrkov
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen R. Marenka 2008-08-08 01:01:31 UTC
gcc -fpic -O1 -c postreload.c

Dropping optimization from -O1 to -O0 or dropping -fpic allows this to succeed.

Using built-in specs.
Target: m68k-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Debian 4.3.1-8' --with-bugurl=file:///usr/share/doc/gcc-4.3/README.Bugs --enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr --enable-shared --with-system-zlib --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --enable-nls --with-gxx-include-dir=/usr/include/c++/4.3 --program-suffix=-4.3 --enable-clocale=gnu --enable-libstdcxx-debug --enable-objc-gc --enable-mpfr --disable-libssp --disable-werror --enable-checking=release --build=m68k-linux-gnu --host=m68k-linux-gnu --target=m68k-linux-gnu
Thread model: posix
gcc version 4.3.1 (Debian 4.3.1-8)
COLLECT_GCC_OPTIONS='-save-temps' '-v' '-fpic' '-O1' '-c' '-m68020'
 /usr/lib/gcc/m68k-linux-gnu/4.3.1/cc1 -E -quiet -v postreload.c -m68020 -fpic -O1 -fpch-preprocess -o postreload.i
ignoring nonexistent directory "/usr/local/include/m68k-linux-gnu"
ignoring nonexistent directory "/usr/lib/gcc/m68k-linux-gnu/4.3.1/../../../../m68k-linux-gnu/include"
ignoring nonexistent directory "/usr/include/m68k-linux-gnu"
#include "..." search starts here:
#include <...> search starts here:
 /usr/local/include
 /usr/lib/gcc/m68k-linux-gnu/4.3.1/include
 /usr/lib/gcc/m68k-linux-gnu/4.3.1/include-fixed
 /usr/include
End of search list.
COLLECT_GCC_OPTIONS='-save-temps' '-v' '-fpic' '-O1' '-c' '-m68020'
 /usr/lib/gcc/m68k-linux-gnu/4.3.1/cc1 -fpreprocessed postreload.i -quiet -dumpbase postreload.c -m68020 -auxbase postreload -O1 -version -fpic -o postreload.s
GNU C (Debian 4.3.1-8) version 4.3.1 (m68k-linux-gnu)
        compiled by GNU C version 4.3.1, GMP version 4.2.2, MPFR version 2.3.1.
GGC heuristics: --param ggc-min-expand=82 --param ggc-min-heapsize=98603
Compiler executable checksum: 941413abde0b2d7fe947ae106f766200
postreload.c: In function 'do_termsform':
postreload.c:745: warning: assignment makes pointer from integer without a cast
postreload.c:806: warning: initialization makes pointer from integer without a cast
postreload.c:842: warning: passing argument 1 of 'Rf_install' makes pointer from integer without a cast
postreload.c:845: warning: passing argument 1 of 'Rf_install' makes pointer from integer without a cast
postreload.c:845: warning: assignment makes pointer from integer without a cast
postreload.c: At top level:
postreload.c:488: warning: 'EncodeVars' used but never defined
postreload.c:533: warning: 'ExpandDots' used but never defined
postreload.c: In function 'do_termsform':
postreload.c:886: error: insn does not satisfy its constraints:
(insn 309 2675 2677 36 postreload.c:446 (set (reg:SI 0 %d0)
        (plus:SI (mem/f:SI (reg:SI 8 %a0) [0 S4 A16])
            (reg:SI 0 %d0))) 132 {*addsi3_internal} (nil))
postreload.c:886: internal compiler error: in reload_cse_simplify_operands, at postreload.c:395
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-4.3/README.Bugs> for instructions.
Comment 1 Stephen R. Marenka 2008-08-08 01:02:02 UTC
Created attachment 16040 [details]
Preprocessed file
Comment 2 Stephen R. Marenka 2008-08-08 01:02:32 UTC
Created attachment 16041 [details]
reduced source from r-base
Comment 3 Andreas Schwab 2008-09-11 15:19:56 UTC
Caused by:

2007-07-23  Peter Bergner  <bergner@vnet.ibm.com>
	    Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/PR28690

Can be reproduced with gcc.c-torture/execute/20060420-1.c when compiled with -O2.
Comment 4 Paolo Bonzini 2008-09-11 15:29:06 UTC
I think there is a missing constrain_operands somewhere, because in

(define_insn "*addsi3_internal"
  [(set (match_operand:SI 0 "nonimmediate_operand" "=m,?a,?a,d,a")
        (plus:SI (match_operand:SI 1 "general_operand" "%0,a,rJK,0,0")
                 (match_operand:SI 2 "general_src_operand" "dIKLT,rJK,a,mSrIKLT,mSrIKLs")))]

the insn does match the fourth alternative with operands 1 and 2 commuted.
Comment 5 Andreas Schwab 2008-09-16 21:00:37 UTC
Reverting this change in commutative_operand_precedence fixes the testcase:

     case RTX_OBJ:
       /* Complex expressions should be the first, so decrease priority
-         of objects.  */
-      return -1;
+         of objects.  Prefer pointer objects over non pointer objects.  */
+      if ((REG_P (op) && REG_POINTER (op))
+	  || (MEM_P (op) && MEM_POINTER (op)))
+	return -1;
+      return -2;
 
     case RTX_COMM_ARITH:
       /* Prefer operands that are themselves commutative to be first.
Comment 6 Paolo Bonzini 2008-09-16 21:02:37 UTC
But that was the meat of fixing PR28690. :-(

The insn should satisfy the constraints of alternative 4.
Comment 7 Richard Biener 2008-09-20 14:29:28 UTC
m68k-linux-gnu is neither primary nor secondary target.
Comment 8 Maxim Kuvyrkov 2009-06-23 11:39:59 UTC
(In reply to comment #6)
> But that was the meat of fixing PR28690. :-(
> 
> The insn should satisfy the constraints of alternative 4.

Well, not really.  For the insn to match alternative 4 the pattern should be non-canonical.  The last sentence in GCC Internals' canonicalization rules says:

"Further canonicalization rules are defined in the function commutative_operand_ 
precedence in ‘gcc/rtlanal.c’"

Unfortunately, commutative_operand_precendence() at the moment clearly states that a pointer (being that a MEM or a REG) has precedence over other RTX_OBJs.  It is absolutely unclear to me why a pointer should have precedence over, say, multiplication or anything else (and "yes", I've read PR28690).

With my target-independent hat on, I would remove that PPC-specific hunk from commutative_operand_precendence() or, if that is really that important to PPC, add a new target hook so that different targets can enjoy privilege of defining that to whatever they seem fit.  Adding such an obscure canonicalization rule for all targets seems unjustified.

I'd like to get some feedback on the above before I start implementing new target hook to make all targets happy.

Peter, I'm CCing you as the author of the commutative_operand_precendence() piece  to get your opinion on the above.
Comment 9 Paolo Bonzini 2009-06-23 12:04:58 UTC
But % makes it commutative, no?  So operand 2 matches operand 0, and operand 1 matches mSrIKLT.
Comment 10 Maxim Kuvyrkov 2009-06-23 12:26:14 UTC
(In reply to comment #9)
> But % makes it commutative, no?

Yes, but that only means that the operands can be swapped *if* swap_commutative_operands_p() returns true.  Due to the funny precedence that does not happen.

> So operand 2 matches operand 0, and operand 1
> matches mSrIKLT.

Matching procedures do not take commutativeness into account.

Part of the problem are optimizations using commutativeness during reload.  One way to paper over the issue is to forbid such optimizations during reload, but that may worsen code.  And this approach is not as clean as letting backends decide if their .md files can handle funny canonicalization rules.
Comment 11 Paolo Bonzini 2009-06-23 13:09:30 UTC
> Yes, but that only means that the operands can be swapped *if*
> swap_commutative_operands_p() returns true.  Due to the funny precedence that
> does not happen.
>
> Matching procedures do not take commutativeness into account.

That would mean that PR28690 screwed up two-address machines big.  In that case my opinion of the patch would change.

Still, I don't think a target hook is the solution.  Even if it adds hack over hack, having the "funny precedence" rules only before reload could be a solution.

I'm pretty sure that we are talking about two different things. :-)
Comment 12 Maxim Kuvyrkov 2009-06-23 17:21:56 UTC
(In reply to comment #11)
> Still, I don't think a target hook is the solution.  Even if it adds hack over
> hack, having the "funny precedence" rules only before reload could be a
> solution.

For the record, I consider the hook to be a feature, the one which backends can use to tweak code generation, not a hack.  At the moment, the precedence which favors pointers is a hack, but we cannot remove it due to powerpc being a important platform.  Also, favoring the pointers may be useful for other similar to ppc platforms even if we are not aware of them right now.

That said, conditioning the precedence on (!reload_in_progress && !reload_completed) fixes the bug so I consider this to be the second best thing to a hook.

It is not as good as a hook because there may be corner cases during reload when pointers should be favored on powerpc (multi-dimension array references, probably).  On the other hand m68k need unrestricted (in the sense that all RTX_OBJ are the same) commutativeness during reload to avoid ICEs.

So, conditioning pointer precedence on reload_in_progress has possible performance degradation on powerpc on one hand and ICE on m68k on the other.

> I'm pretty sure that we are talking about two different things. :-)

Err, I don't fully follow you here.  What are the two different things?
Comment 13 Maxim Kuvyrkov 2009-06-24 16:02:10 UTC
Created attachment 18061 [details]
Proposed patch

Here is a patch moving precedence handling of pointers to powerpc backend.
Comment 14 Richard Biener 2009-08-04 12:29:23 UTC
GCC 4.3.4 is being released, adjusting target milestone.
Comment 15 Maxim Kuvyrkov 2009-08-04 13:14:36 UTC
There are several (4, I think) patches posted in gcc-patches@ for this bug.  A reload/recog maintainer is needed to choose the most appropriate one.
Comment 16 Peter Bergner 2009-08-04 13:35:58 UTC
There have been many patches posted, but most have caused serious performance degradations on power.  However, the two latest patches to reload do not.  They are:

 1) http://gcc.gnu.org/ml/gcc-patches/2009-07/msg00816.html
 2) http://gcc.gnu.org/ml/gcc-patches/2009-07/msg00823.html

Maxim, have you tried either of these patches and do they work for you?

Uli, can you please have a look at Richard's and Paolo's patches and does one or the other seem like a "better" fix?
Comment 17 Maxim Kuvyrkov 2009-08-04 13:43:42 UTC
I'll try the above two patches and will report in a couple of days.
Comment 18 Ulrich Weigand 2009-08-05 14:59:45 UTC
(In reply to comment #16)
> Uli, can you please have a look at Richard's and Paolo's patches and does one
> or the other seem like a "better" fix?

I've yet another suggestion :-)   See my message at:
http://gcc.gnu.org/ml/gcc-patches/2009-08/msg00254.html
Comment 19 Ulrich Weigand 2009-08-10 15:34:23 UTC
Subject: Bug 37053

Author: uweigand
Date: Mon Aug 10 15:34:09 2009
New Revision: 150626

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=150626
Log:
	PR target/37053
	* reload1.c (reload_as_needed): Use cancel_changes to completely
	undo a failed replacement attempt.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/reload1.c

Comment 20 Andreas Schwab 2009-08-23 18:19:32 UTC
Fixed in 4.5.0.
Comment 21 Paolo Bonzini 2009-12-29 19:30:12 UTC
Reopening since it is still broken on the other open branches.
Comment 22 Richard Biener 2010-05-22 18:12:33 UTC
GCC 4.3.5 is being released, adjusting target milestone.
Comment 23 Thorsten Glaser 2010-11-01 16:10:32 UTC
Backported r150626 from Comment 19 and applied to Debian/m68k gcc-4.4
(native). Fixes this ICE when building libjpeg for me. No regression
tests run, though.
Comment 24 Paolo Bonzini 2010-11-01 16:38:53 UTC
You'd need also the patch for bug 41064.
Comment 25 Paolo Bonzini 2010-11-01 16:40:07 UTC
*** Bug 40414 has been marked as a duplicate of this bug. ***
Comment 26 Thorsten Glaser 2010-11-01 16:53:18 UTC
Probably, but not on m68k ;-)
Comment 27 Paolo Bonzini 2010-11-01 16:58:20 UTC
Better: not for this testcase.  We found it on CRIS, but the bug could really happen on any target.
Comment 28 Thorsten Glaser 2010-11-03 21:41:38 UTC
OK, added.
Comment 29 Thorsten Glaser 2010-11-06 16:25:00 UTC
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=602099#20
in case someone’s interested…
Comment 30 Richard Biener 2011-06-27 12:13:51 UTC
4.3 branch is being closed, moving to 4.4.7 target.
Comment 31 Jakub Jelinek 2012-03-13 13:04:11 UTC
Fixed in 4.5+, 4.4 is no longer supported.