PATCH for genrecog.c bug with handling address_operand (was: Re: Committed: fix typo in ns32k.md ...)

[Hans-Peter Nilsson <hans-peter dot nilsson at axis dot com>]

Hi.

[ Small patch and test-case come last, after this verbosity. ]

I'm trying to make amends for checking in that bogus ns32k.md
change.  My apologies to the both of you using this port. :-)
 On the light side, it covered up enough to get it through
bootstrapping.

I've found that the *real* problem behind the ns32k bootstrap
failure and address_operand use, is a generic bug in genrecog.c

The genrecog code knows that address_operand is special when
it creates the predicate test, but when it forms the decision
tree later, it handles address_operand like any other predicate.
This causes insns using address_operand as below (seemingly
mismatching modes) not to be recognized correctly.

For ns32k.md we used to have, before my bogus change:

(define_insn ""
  [(set (match_operand:SI 0 "general_operand" "=g<")
	(match_operand:QI 1 "address_operand" "p"))]
  ""
  "...")

To check the operand1 branch against other insn alternatives,
genrecog generates tests that compare the modes (fast), then
checks the predicate via PREDICATE_CODES, then via the predicate
(slower).
 There is (correctly) no mode test generated for the operand1
address_operand above.  There *is* a mode test for operand1 of
the movsi2 insn.  But when the decision trees for insns are
merged, the mode-test for the movsi2 operand1 is compared to the
predicate test, and the "mismatching" modes cause a decision not
to emit code to check back again, if movsi2 (and others) fail.

Without the patch, we get an insn-recog.c for ns32k.md (before
my Jan 9 change) that looks like:

 ...
 L815: ATTRIBUTE_UNUSED_LABEL
  x1 = XEXP (x0, 1);
  if (GET_MODE (x1) == SImode)
    goto L1421;
  if (address_operand (x1, QImode))
    {
      operands[1] = x1;
      return 158;
    }
  x1 = XEXP (x0, 0);
  goto L1095;
 ...

So the address_operand check is never tested for an SImode
operand, and there's no backtracking to check it after the
tests at L1421.  (Repeat: the QImode argument to address_operand
is *not* for the mode of the address (SImode), but to test that
the address can *point* to something with this mode.)

With the patch, we get (including one more chunk of code to
illustrate the backtracking):

 ...
 L815: ATTRIBUTE_UNUSED_LABEL
  x1 = XEXP (x0, 1);
  if (GET_MODE (x1) == SImode)
    goto L1421;
 L800: ATTRIBUTE_UNUSED_LABEL
  if (address_operand (x1, QImode))
    {
      operands[1] = x1;
      return 158;
    }
  x1 = XEXP (x0, 0);
  goto L1095;

 L1421: ATTRIBUTE_UNUSED_LABEL
  switch (GET_CODE (x1))
    {
    case XOR:
      goto L816;
    case ZERO_EXTRACT:
      goto L837;
    default:
     break;
   }
  goto L800;
 ...
 (more similar code with "goto L800")

Note that there's a label L800 added after the SImode check,
before the address_operand check.  The code snippet shows how it
is used to check for an address_operand, if for example, the XOR
and ZERO_EXTRACT tests fail.

Except for the dusty ns32k, other more-or-less dusty ISA:s have
similar constructs with "mismatching modes" on
address_operands, and are most probably also affected.  I found
fx80, gmicro, m68k, m88k, arm, pyramid ("pyr"), vax and tahoe
when grepping for address_operand and mode != Pmode where it
matters.

The ia32 and sparc would not really be affected, but those are
the ones "closest" to me, so that's where I've bootstrapped the
patch.  Sort of "I'll look here where there's more light" :-}
FWIW, insn-recog.c was different on sparc, not on ia32.

The reason why that "lea"-like insn above must be matched, is a
suboptimal (but not incorrect) constraint for the ns32k movstrsi
recognizer pattern, causing reload to reject it for reloading
into a general operand.  Many address_operands are not
general_operands.  I hope to get back to the suboptimal
contraints issue in another message.


Running "cc1 -O2" for ns32k-pc532-mach as above, with my Jan 9
change to ns32k.md reverted, fails with the test-case below.
 I have distilled it from cexp.i (mostly bison.simple).
Should I install this test-case?

Anyway, is this patch OK to commit?

If so, is there a reason why I should not revert my Jan 9
ns32k.md change?
 Admittedly, the mode for address_operand does not matter,
because the ns32k GO_IF_LEGITIMATE_ADDRESS does not use the
mode.  But reverting a patch for a non-error is more logical
than keeping it.

Wed Jan 12 23:12:47 2000  Hans-Peter Nilsson  <hp@axis.com>

	* genrecog.c (maybe_both_true_2): Do not compare a predicate-test
	to a mode-test, if the predicate is address_operand.

Index: genrecog.c
===================================================================
RCS file: /cvs/gcc/egcs/gcc/genrecog.c,v
retrieving revision 1.72
diff -p -c -r1.72 genrecog.c
*** genrecog.c	2000/01/09 14:23:35	1.72
--- genrecog.c	2000/01/12 22:27:27
***************
*** 1,5 ****
  /* Generate code from machine description to recognize rtl as insns.
!    Copyright (C) 1987, 88, 92-95, 97-98, 1999 Free Software Foundation, Inc.
  
     This file is part of GNU CC.
  
--- 1,5 ----
  /* Generate code from machine description to recognize rtl as insns.
!    Copyright (C) 1987, 88, 92-95, 97-99, 2000 Free Software Foundation, Inc.
  
     This file is part of GNU CC.
  
*************** maybe_both_true_2 (d1, d2)
*** 1012,1018 ****
  	{
  	  if (d2->type == DT_mode)
  	    {
! 	      if (d1->u.pred.mode != d2->u.mode)
  		return 0;
  	    }
  	  /* Don't check two predicate modes here, because if both predicates
--- 1012,1023 ----
  	{
  	  if (d2->type == DT_mode)
  	    {
! 	      if (d1->u.pred.mode != d2->u.mode
! 		  /* The mode of an address_operand predicate is the
! 		     mode of the memory, not the operand.  It can only
! 		     be used for testing the predicate, so we must
! 		     ignore it here.  */
! 		  && strcmp (d1->u.pred.name, "address_operand") != 0)
  		return 0;
  	    }
  	  /* Don't check two predicate modes here, because if both predicates

*** /dev/null	Tue Jan  1 05:00:00 1980
--- testsuite/gcc.c-torture/compile/20000112-1.c	Thu Jan 13 00:23:06 2000
***************
*** 0 ****
--- 1,72 ----
+ typedef union {
+   struct constant {long long  value; int signedp;} integer;
+ } YYSTYPE;
+ extern const short yyr2[];
+ extern const short yypact[];
+ int yyparse (void);
+ int
+ yyparse( )
+ {
+   register int yystate;
+   register int yyn;
+   register short *yyssp;
+   register YYSTYPE *yyvsp;
+   short	yyssa[200 ];	 
+   YYSTYPE yyvsa[200 ];	 
+   short *yyss = yyssa;		 
+   YYSTYPE *yyvs = yyvsa;	 
+   int yystacksize = 200 ;
+   YYSTYPE yyval;		 
+   int yylen;
+   yystate = 0;
+   yyssp = yyss - 1;
+   yyvsp = yyvs;
+ yynewstate:
+   *++yyssp = yystate;
+   if (yyssp >= yyss + yystacksize - 1)
+     {
+       YYSTYPE *yyvs1 = yyvs;
+       short *yyss1 = yyss;
+       int size = yyssp - yyss + 1;
+       if (yystacksize >= 10000 )
+ 	{
+ 	  yyerror("parser stack overflow");
+ 	  return 2;
+ 	}
+       yystacksize *= 2;
+       if (yystacksize > 10000 )
+ 	yystacksize = 10000 ;
+       yyss = (short *) __builtin_alloca(  yystacksize * sizeof (*yyssp)  )  ;
+       __builtin_memcpy( (char *)yyss ,  (char *)yyss1 ,  size * sizeof (*yyssp) ) ;
+       yyvs = (YYSTYPE *) __builtin_alloca(  yystacksize * sizeof (*yyvsp)  )  ;
+       __builtin_memcpy( (char *)yyvs ,  (char *)yyvs1 ,  size * sizeof (*yyvsp) ) ;
+       yyssp = yyss + size - 1;
+       yyvsp = yyvs + size - 1;
+       if (yyssp >= yyss + yystacksize - 1)
+ 	return(1) ;
+     }
+   goto yybackup;
+  yybackup:
+   yyn = yypact[yystate];
+   if (yyn == -32768 )
+     goto yydefault;
+   if (yyn < 0 || yyn > 189)
+     goto yydefault;
+   else if (yyn == 0)
+     goto yynewstate;
+   if (yyn == 77 )
+     return(0) ;
+   yystate = yyn;
+   goto yynewstate;
+ yydefault:
+   if (yyn == 0)
+     goto yynewstate;
+ yyreduce:
+   yylen = yyr2[yyn];
+   if (yylen > 0)
+     yyval = yyvsp[1-yylen];  
+   yyvsp -= yylen;
+   yyssp -= yylen;
+   *++yyvsp = yyval;
+   goto yynewstate;
+ }

brgds, H-P