Bug 27661 - ICE in subst_reloads
Summary: ICE in subst_reloads
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 4.1.0
: P3 normal
Target Milestone: 4.2.0
Assignee: Not yet assigned to anyone
URL:
Keywords: ice-on-valid-code
Depends on:
Blocks:
 
Reported: 2006-05-18 15:55 UTC by Michael Matz
Modified: 2006-07-20 11:16 UTC (History)
4 users (show)

See Also:
Host: s390x-ibm-linux
Target: s390x-ibm-linux
Build:
Known to work:
Known to fail:
Last reconfirmed: 2006-05-18 16:59:37


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Matz 2006-05-18 15:55:49 UTC
The following testcase produces an ICE in subst_reloads, when compiled
with "-march=z990 -O2 -ftracer":

----------------snip--------------------
extern int memcmp(const void *s1, const void *s2, unsigned long n);
extern int printf (__const char *__restrict __format, ...);
typedef struct
{
   char tmp[4092];
   char msglen[4];
   char msgtype[2];
} TESTSTRUCT;
void test_func(TESTSTRUCT *testtb) {
    if (testtb)
        printf("a");
    if (memcmp(testtb->msgtype, "a", 2))
        printf("a");
    printf("b");
}
------------------snap--------------------

% ./gcc/cc1 -march=z990 -O2 -ftracer test.i
 test_func
Analyzing compilation unitPerforming intraprocedural optimizations
Assembling functions:
 test_func
test.i: In function ‘test_func’:
test.i:19: internal compiler error: in subst_reloads, at reload.c:6108

This already ICEs with the hammer branch, and I think is also still a latent
problem in trunk.  It's a combination of the tracer and CSE working hand in 
hand to produce an invalid MEM address.  It goes like so:

1) Initially the memcmp() call is implemented by the cmpmem_short instruction
   of s390.  Remember that with -march=z990 we have long displacements active.
   The instruction emitted for the memcmp() is:
(insn 69 68 70 1 (parallel [
            (set (reg:CCU 33 %cc)
                (compare:CCU (mem/s:BLK (reg/f:DI 46) [0 S2 A8])
                    (mem/s:BLK (plus:DI (reg/v/f:DI 43 [ testtb ])
                            (const_int 4096 [0x1000]))
                            [0 <variable>.msgtype+0 S2 A8])))
            (use (const_int 1 [0x1]))
            (use (const:BLK (unspec:BLK [
                            (const_int 0 [0x0])
                        ] 213)))
            (clobber (scratch:DI))
        ]) 92 {*cmpmem_short} (nil)
    (expr_list:REG_EQUAL (compare:CCU (mem/s:BLK (symbol_ref/f:DI ("*.LC0") [flags 0x2] <string_cst 0x20000368b70>) [0 S2 A8])
            (mem/s:BLK (plus:DI (reg/v/f:DI 43 [ testtb ])
                    (const_int 4096 [0x1000]))
                      [0 <variable>.msgtype+0 S2 A8]))
        (nil)))

  So, everything fine, but note the (mem (plus (reg 43) (4096)) operand, which
  we are going to screw up soon.

2) Then -ftracer comes along an duplicates a trace through the whole function
   using the "if (testtb)" predicate.  So we end up with two copies of the
   body, including the above instruction.  One of them under the testtb==0
   branch.

3) Now CSE2 comes into play, with it's path following feature.  In the second
   version of the above insn it knows that testtb (== pseudo 43) is zero.
   Hence it happily changes the insn to read:
(insn 24 23 25 3 (parallel [
            (set (reg:CCU 33 %cc)
                (compare:CCU (mem/s:BLK (reg/f:DI 46) [0 S2 A8])
                    (mem/s:BLK (const_int 4096 [0x1000])
                      [0 <variable>.msgtype+0 S2 A8])))
            (use (const_int 1 [0x1]))
            (use (const:BLK (unspec:BLK [
                            (const_int 0 [0x0])
                        ] 213)))
            (clobber (scratch:DI))
        ]) 92 {*cmpmem_short} (nil)

   Note how the operand in question now is (mem (const_int 4096)).  That is
   fine so far normally, as the compiler can't know that this is going to
   be in invalid address operand.  It can't know that because all predicates
   say that it is valid.

4) reload comes over that insn, looks at the various operands and addresses
   and also determines that (mem (const_int 4096)) is valid because
   strict_memory_address_p returns true for it.

5) But then the constraints are checked too.  And we use 'Q' constraints
   in the cmpmem_short insn:
  (define_insn "*cmpmem_short"
  [(set (reg:CCU CC_REGNUM)
        (compare:CCU (match_operand:BLK 0 "memory_operand" "Q,Q,Q")
                     (match_operand:BLK 1 "memory_operand" "Q,Q,Q")))
   (use (match_operand 2 "nonmemory_operand" "n,a,a"))
   (use (match_operand 3 "immediate_operand" "X,R,X"))
   (clobber (match_scratch 4 "=X,X,&a"))]
   ...
   And the 'Q' constraint forbids long displacements also on long disp
   machines.  So we now have an inconsistent situation.  As the constraints
   don't match we try to reload this address operand (const_int 4096):

Reloads for insn # 24
Reload 0: reload_in (VOID) = (const_int 4096 [0x1000])
        ADDR_REGS, RELOAD_FOR_INPUT (opnum = 1)
        reload_in_reg: (const_int 4096 [0x1000])

   But this operand can't be reloaded easily, because the const_int has no
   mode, hence this whole reload has only VOIDmode, and no register can
   be found for it.  Actually this situation should never happen (i.e. a
   modeless reload), that's why it isn't handled in push_reload and friends.
   All these invalid addresses should have been created a reload for much
   earlier than in the constraint checking.

I unfortunately don't have a good idea how to fix this.  I think these
large displacements much somehow be forbidden already in the predicate,
not only via the constraints, which would mean that one can't use
"memory_operand" as predicate.  Or cmpmem_short needs to accept also the 'S'
constraint.  Or some way would be found for reload to determine a mode for
this reload despite it being only a const_int.  Pmode would be natural.
Alternatively strict_memory_address_p() (via legitimate_address_p() in s390.c)
could reject addresses which only consist of a displacement larger than
4096.  By that long displacement would still be accepted everywhere (when the
machine has them) but only when connected with some register as base or index.
That would ensure that a mode is available for the whole address which makes
reloading possible.
Comment 1 Michael Matz 2006-05-18 16:07:06 UTC
Like so perhaps.  It fixes the ICE for me, but I've done no further testing:

--- s390.c.orig 2006-05-18 18:05:59.442621578 +0200
+++ config/s390/s390.c  2006-05-18 18:05:34.632630936 +0200
@@ -2731,6 +2731,9 @@ legitimate_address_p (enum machine_mode
        return false;
       if (ad.indx && !REG_OK_FOR_INDEX_STRICT_P (ad.indx))
        return false;
+      if (!ad.base && !ad.indx
+          && !s390_short_displacement (ad.disp))
+       return false;
     }
   else
     {
Comment 2 Andreas Krebbel 2006-05-18 16:59:37 UTC
Thanks for your comprehensive debugging!

At first glance I would say reload should take a word_mode register and reload the constant. The code added by your patch to legitimate_address_p says that a single number is not a valid address which isn't true. We also allow reg+reg+constant in legitimate_address_p and rely on reload to fix this for Q and S constraints and I think the same should happen here as well. 
Comment 3 Ulrich Weigand 2006-05-22 13:27:27 UTC
Looking somewhat more into this problem, there are other places where
reload decides to reload an CONST_INT as address.  Where this happens,
it usually uses Pmode as the mode to do the reload in (which makes
sense as Pmode should always be valid as the mode of an address).

Look e.g. at the various call sites of find_reloads_address_part.

However, at the place where the decision to reload a valid address
due to and EXTRA_MEMORY_CONSTRAINT or 'o' constraint is made, this
special treatment of VOIDmode constants is missing.  However, it
looks to me that this was simply an oversight here.

Thus, I'd propose something like the following patch (as of right
now completely untested) to replace VOIDmode by Pmode at that place
too.  This should fix the problem.

Index: gcc/reload.c
===================================================================
*** gcc/reload.c        (revision 113828)
--- gcc/reload.c        (working copy)
*************** find_reloads (rtx insn, int replace, int
*** 3854,3864 ****
                 && goal_alternative_offmemok[i]
                 && MEM_P (recog_data.operand[i]))
          {
            operand_reloadnum[i]
              = push_reload (XEXP (recog_data.operand[i], 0), NULL_RTX,
                             &XEXP (recog_data.operand[i], 0), (rtx*) 0,
                             base_reg_class (VOIDmode, MEM, SCRATCH),
!                            GET_MODE (XEXP (recog_data.operand[i], 0)),
                             VOIDmode, 0, 0, i, RELOAD_FOR_INPUT);
            rld[operand_reloadnum[i]].inc
              = GET_MODE_SIZE (GET_MODE (recog_data.operand[i]));
--- 3854,3872 ----
                 && goal_alternative_offmemok[i]
                 && MEM_P (recog_data.operand[i]))
          {
+           /* If the address to be reloaded is a VOIDmode constant,
+              use Pmode as mode of the reload register, as would have
+              been done by find_reloads_address.  */
+           enum machine_mode address_mode;
+           address_mode = GET_MODE (XEXP (recog_data.operand[i], 0));
+           if (address_mode == VOIDmode)
+             address_mode = Pmode;
+
            operand_reloadnum[i]
              = push_reload (XEXP (recog_data.operand[i], 0), NULL_RTX,
                             &XEXP (recog_data.operand[i], 0), (rtx*) 0,
                             base_reg_class (VOIDmode, MEM, SCRATCH),
!                            address_mode,
                             VOIDmode, 0, 0, i, RELOAD_FOR_INPUT);
            rld[operand_reloadnum[i]].inc
              = GET_MODE_SIZE (GET_MODE (recog_data.operand[i]));


I'll test the patch and propose it on gcc-patches if it works out.
Could you verify this patch helps with your original testcase?
Comment 4 patchapp@dberlin.org 2006-05-24 03:30:26 UTC
Subject: Bug number PR rtl-optimization/27661

A patch for this bug has been added to the patch tracker.
The mailing list url for the patch is http://gcc.gnu.org/ml/gcc-patches/2006-05/msg01175.html
Comment 5 Ulrich Weigand 2006-05-26 20:22:30 UTC
Subject: Bug 27661

Author: uweigand
Date: Fri May 26 20:21:53 2006
New Revision: 114141

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=114141
Log:
	PR rtl-optimization/27661
	* reload.c (find_reloads): When reloading a VOIDmode constant
	as address due to an EXTRA_MEMORY_CONSTRAINT or 'o' constraint,
	use Pmode as mode of the reload register.

	PR rtl-optimization/27661
	* gcc.dg/pr27661.c: New test case.

Added:
    trunk/gcc/testsuite/gcc.dg/pr27661.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/reload.c
    trunk/gcc/testsuite/ChangeLog

Comment 6 Andrew Pinski 2006-07-20 11:16:57 UTC
Fixed.