[RFA:] Adjust documentation for LEGITIMATE_CONSTANT_P et al to match reality, take 2

[RFA:] Adjust documentation for LEGITIMATE_CONSTANT_P et al to match reality, take 2

[Hans-Peter Nilsson]

See the thread at
<http://gcc.gnu.org/ml/gcc-patches/2008-09/msg00552.html>.  This
update merges, with a single s/code/type/ for CONSTANT_P, mine
and Paolo Bonzini's attempts and hopefully addresses most of
Richard Sandiford's objections.  I hope most agree this is a
reasonable step forward and that the discontented not see it as
a step backward.

Tested "make info-gcc dvi-gcc" and inspected output *as well as messages*.
(Protocol says to check that the dvi warnings don't regress when you add
text, for new chapters that often happens.)

Ok to commit?

	* doc/tm.texi (Addressing Modes) <CONSTANT_ADDRESS_P>: Recommend
	using CONSTANT_P in conjunction with CONST contents test.
	<CONSTANT_P, GO_IF_LEGITIMATE_ADDRESS, LEGITIMATE_CONSTANT_P>:
	Ditto.
	(PIC) <LEGITIMATE_PIC_OPERAND_P>: Remove sentence advising to
	leave undefined in the rare special case of all constants being
	valid.

Index: doc/tm.texi
===================================================================
--- doc/tm.texi	(revision 140538)
+++ doc/tm.texi	(working copy)
@@ -5267,17 +5267,23 @@ post-address side-effect generation invo
 
 @defmac CONSTANT_ADDRESS_P (@var{x})
 A C expression that is 1 if the RTX @var{x} is a constant which
-is a valid address.  On most machines, this can be defined as
-@code{CONSTANT_P (@var{x})}, but a few machines are more restrictive
-in which constant addresses are supported.
+is a valid address.  A port can use @code{CONSTANT_P (@var{x})} to
+simplify the expression, but must always validize the contents of
+a @code{CONST} expression.
 @end defmac
 
 @defmac CONSTANT_P (@var{x})
 @code{CONSTANT_P}, which is defined by target-independent code,
-accepts integer-values expressions whose values are not explicitly
+accepts integer-valued expressions whose values are not explicitly
 known, such as @code{symbol_ref}, @code{label_ref}, and @code{high}
 expressions and @code{const} arithmetic expressions, in addition to
 @code{const_int} and @code{const_double} expressions.
+This macro only checks the type of the @var{x} rtx; it does not
+ensure that the target supports using @var{x} as an immediate
+value or as a literal data constant.  In particular, the target
+might use a constant pool for large constants or might split them,
+or @var{x} might be a @code{CONST} wrapping an expression that is
+not valid for the target.
 @end defmac
 
 @defmac MAX_REGS_PER_ADDRESS
@@ -5321,9 +5327,8 @@ levels of macros may be the same whether
 
 Normally, constant addresses which are the sum of a @code{symbol_ref}
 and an integer are stored inside a @code{const} RTX to mark them as
-constant.  Therefore, there is no need to recognize such sums
-specifically as legitimate addresses.  Normally you would simply
-recognize any @code{const} as legitimate.
+constant, but this is not always the case.  Therefore, you need to
+validize such expressions as legitimate addresses.
 
 Usually @code{PRINT_OPERAND_ADDRESS} is not prepared to handle constant
 sums that are not marked with  @code{const}.  It assumes that a naked
@@ -5457,9 +5462,8 @@ You may assume that @var{addr} is a vali
 @defmac LEGITIMATE_CONSTANT_P (@var{x})
 A C expression that is nonzero if @var{x} is a legitimate constant for
 an immediate operand on the target machine.  You can assume that
-@var{x} satisfies @code{CONSTANT_P}, so you need not check this.  In fact,
-@samp{1} is a suitable definition for this macro on machines where
-anything @code{CONSTANT_P} is valid.
+@var{x} satisfies @code{CONSTANT_P}, so you need not check this, but
+you need to validize @code{CONST} expressions.
 @end defmac
 
 @deftypefn {Target Hook} rtx TARGET_DELEGITIMIZE_ADDRESS (rtx @var{x})
@@ -6806,9 +6810,7 @@ A C expression that is nonzero if @var{x
 operand on the target machine when generating position independent code.
 You can assume that @var{x} satisfies @code{CONSTANT_P}, so you need not
 check this.  You can also assume @var{flag_pic} is true, so you need not
-check it either.  You need not define this macro if all constants
-(including @code{SYMBOL_REF}) can be immediate operands when generating
-position independent code.
+check it either.
 @end defmac
 
 @node Assembler Format

brgds, H-P

Re: [RFA:] Adjust documentation for LEGITIMATE_CONSTANT_P et al to match reality, take 2

[Richard Sandiford]

Hans-Peter Nilsson <hans-peter.nilsson@axis.com> writes:
> See the thread at
> <http://gcc.gnu.org/ml/gcc-patches/2008-09/msg00552.html>.  This
> update merges, with a single s/code/type/ for CONSTANT_P, mine
> and Paolo Bonzini's attempts and hopefully addresses most of
> Richard Sandiford's objections.  I hope most agree this is a
> reasonable step forward and that the discontented not see it as
> a step backward.

Can we wait until we know which way 4.4 goes wrt (const (minus ...))?
If we go for something like the grammar I suggested, this:

>  @defmac CONSTANT_ADDRESS_P (@var{x})
>  A C expression that is 1 if the RTX @var{x} is a constant which
> -is a valid address.  On most machines, this can be defined as
> -@code{CONSTANT_P (@var{x})}, but a few machines are more restrictive
> -in which constant addresses are supported.
> +is a valid address.  A port can use @code{CONSTANT_P (@var{x})} to
> +simplify the expression, but must always validize the contents of
> +a @code{CONST} expression.
>  @end defmac

wouldn't always be necessary on CISCy targets, just as it traditionally
hasn't been in the past.

I don't think it's productive to argue over the fine detail until we
know what 4.4 does.

Richard

Re: [RFA:] Adjust documentation for LEGITIMATE_CONSTANT_P et al to match reality, take 2

[Hans-Peter Nilsson]

> From: Richard Sandiford <rdsandiford@googlemail.com>
> Date: Tue, 23 Sep 2008 20:22:46 +0100

> Hans-Peter Nilsson <hans-peter.nilsson@axis.com> writes:
> > See the thread at
> > <http://gcc.gnu.org/ml/gcc-patches/2008-09/msg00552.html>.  This
> > update merges, with a single s/code/type/ for CONSTANT_P, mine
> > and Paolo Bonzini's attempts and hopefully addresses most of
> > Richard Sandiford's objections.  I hope most agree this is a
> > reasonable step forward and that the discontented not see it as
> > a step backward.
> 
> Can we wait until we know which way 4.4 goes wrt (const (minus ...))?
> If we go for something like the grammar I suggested, this:

I've waited, and I think we know now: it does just the same.
It's stage 3 and I don't see any progress on your suggested
rules, so it seems we're going nowhere.

> > +is a valid address.  A port can use @code{CONSTANT_P (@var{x})} to
> > +simplify the expression, but must always validize the contents of
> > +a @code{CONST} expression.
> >  @end defmac
> 
> wouldn't always be necessary on CISCy targets, just as it traditionally
> hasn't been in the past.
> 
> I don't think it's productive to argue over the fine detail until we
> know what 4.4 does.

What fine detail?  What is it that we don't know?  I see
long-standing regression failures for my target.  As it stands,
it is necessary to do what my documentation change suggests, and
documenting it would be an improvement over status quo.  Why
can't I document what the code does, even in the suggested vague
terms?  Can't we do that, and *then* document and enforce your
rules?

You're the one protesting this and asking to wait, but I don't
see any patches along that route.  One would be quite welcomed.
How about a patch to tm.texi documenting your suggestions, for
starters?

Or should we remove the expicit const minus generation in the
middle end and look the other way wrt. CONST rules?  I'm fine
with that too.  Maybe such a change wouldn't even break ppc
anymore...

brgds, H-P

Re: [RFA:] Adjust documentation for LEGITIMATE_CONSTANT_P et al to match reality, take 2

[Richard Sandiford]

Hans-Peter Nilsson <hans-peter.nilsson@axis.com> writes:
>> From: Richard Sandiford <rdsandiford@googlemail.com>
>> Date: Tue, 23 Sep 2008 20:22:46 +0100
>
>> Hans-Peter Nilsson <hans-peter.nilsson@axis.com> writes:
>> > See the thread at
>> > <http://gcc.gnu.org/ml/gcc-patches/2008-09/msg00552.html>.  This
>> > update merges, with a single s/code/type/ for CONSTANT_P, mine
>> > and Paolo Bonzini's attempts and hopefully addresses most of
>> > Richard Sandiford's objections.  I hope most agree this is a
>> > reasonable step forward and that the discontented not see it as
>> > a step backward.
>> 
>> Can we wait until we know which way 4.4 goes wrt (const (minus ...))?
>> If we go for something like the grammar I suggested, this:
>
> I've waited, and I think we know now: it does just the same.
> It's stage 3 and I don't see any progress on your suggested
> rules, so it seems we're going nowhere.
>
>> > +is a valid address.  A port can use @code{CONSTANT_P (@var{x})} to
>> > +simplify the expression, but must always validize the contents of
>> > +a @code{CONST} expression.
>> >  @end defmac
>> 
>> wouldn't always be necessary on CISCy targets, just as it traditionally
>> hasn't been in the past.
>> 
>> I don't think it's productive to argue over the fine detail until we
>> know what 4.4 does.
>
> What fine detail?  What is it that we don't know?  I see
> long-standing regression failures for my target.  As it stands,
> it is necessary to do what my documentation change suggests, and
> documenting it would be an improvement over status quo.  Why
> can't I document what the code does, even in the suggested vague
> terms?  Can't we do that, and *then* document and enforce your
> rules?
>
> You're the one protesting this and asking to wait, but I don't
> see any patches along that route.  One would be quite welcomed.
> How about a patch to tm.texi documenting your suggestions, for
> starters?
>
> Or should we remove the expicit const minus generation in the
> middle end and look the other way wrt. CONST rules?  I'm fine
> with that too.  Maybe such a change wouldn't even break ppc
> anymore...

The tone of this thread seems to be degenerating, so I'm a little
hesitant to respond, but: removing the const minus generation from
the middle-end is indeed the idea.  It's waiting for a Darwin
maintainer to review the Darwin patch; every other pre-requisite
has been approved.

To be clear, I think using UNSPECs instead of const minus is a step
forward in its own right, and would be even if the const minus stuff
in simplify-rtx.c weren't needed.  See the covering messages for
details, including an unwanted canonicalisation we get when simplifying
(minus X Y) - 1.

If that's approved, I think there would be nothing to stop us removing
the const minus code, which again seems like an improvement in its
own right.  The code handles a very special case, and isn't consistent
with routines like plus_constant.

With the const minus simplifications gone, do you know of any other code
that would generate unexpected CONSTs?

Richard

Re: [RFA:] Adjust documentation for LEGITIMATE_CONSTANT_P et al to match reality, take 2

[Hans-Peter Nilsson]

> From: Richard Sandiford <rdsandiford@googlemail.com>
> Date: Sun, 05 Oct 2008 21:44:25 +0100

> removing the const minus generation from
> the middle-end is indeed the idea.  It's waiting for a Darwin
> maintainer to review the Darwin patch; every other pre-requisite
> has been approved.

Thank you for staying with this.

> With the const minus simplifications gone, do you know of any other code
> that would generate unexpected CONSTs?

No.  Though, there is the confusing comment in cse.c:fold_rtx
line 3189, which might be taken as such code being legitimate
and more such code being introduced:

	/* NEG of PLUS could be converted into MINUS, but that causes
	   expressions of the form
	   (CONST (MINUS (CONST_INT) (SYMBOL_REF)))
	   which many ports mistakenly treat as LEGITIMATE_CONSTANT_P.
	   FIXME: those ports should be fixed.  */

brgds, H-P

Re: [RFA:] Adjust documentation for LEGITIMATE_CONSTANT_P et al to match reality, take 2

[Richard Sandiford]

Hans-Peter Nilsson <hans-peter.nilsson@axis.com> writes:
>> From: Richard Sandiford <rdsandiford@googlemail.com>
>> Date: Sun, 05 Oct 2008 21:44:25 +0100
>
>> With the const minus simplifications gone, do you know of any other code
>> that would generate unexpected CONSTs?
>
> No.  Though, there is the confusing comment in cse.c:fold_rtx
> line 3189, which might be taken as such code being legitimate
> and more such code being introduced:
>
> 	/* NEG of PLUS could be converted into MINUS, but that causes
> 	   expressions of the form
> 	   (CONST (MINUS (CONST_INT) (SYMBOL_REF)))
> 	   which many ports mistakenly treat as LEGITIMATE_CONSTANT_P.
> 	   FIXME: those ports should be fixed.  */

Hmm, good catch, thanks.  When I submit the "CONST grammar" patch,
I'll remove that at the same time.

Richard

Re: [RFA:] Adjust documentation for LEGITIMATE_CONSTANT_P et al to match reality, take 2

[Richard Sandiford]

Here's the patch to remove the (const (minus ...)) code.
Bootstrapped & regression-tested on x86_64-linux-gnu.  Also tested
by comparing the before and after assembly output for:

  gcc.c-torture gcc.dg g++.dg

on these targets:

  powerpc-darwin powerpc64-darwin i386-darwin x86_64-darwin
  powerpc-linux-gnu powerpc64-linux-gnu powerpc-ibm-aix5.3.0

using the options "-O2 -fpic".  The output was the same.

However, I grepped for all gen_rtx_CONSTs in gcc/, and I noticed
that there are still some dodgy cases in arm, m68hc11, s390 and sh.
(m68hc11's only transgression is to wrap a bare symbol_ref in a const;
it doesn't use minus.)  So I'd better fix those before submitting the
patch properly.

For reference, I've also attached a cse patch (not yet tested)
and a patch to the const documentation.

Richard


Index: gcc/gcc/simplify-rtx.c
===================================================================
--- gcc.orig/gcc/simplify-rtx.c	2008-10-08 05:15:29.000000000 +0100
+++ gcc/gcc/simplify-rtx.c	2008-10-08 19:53:13.000000000 +0100
@@ -3678,24 +3678,6 @@ simplify_plus_minus (enum rtx_code code,
      one CONST_INT, and the sort will have ensured that it is last
      in the array and that any other constant will be next-to-last.  */
 
-  if (GET_CODE (ops[n_ops - 1].op) == CONST_INT)
-    i = n_ops - 2;
-  else
-    i = n_ops - 1;
-
-  if (i >= 1
-      && ops[i].neg
-      && !ops[i - 1].neg
-      && CONSTANT_P (ops[i].op)
-      && GET_CODE (ops[i].op) == GET_CODE (ops[i - 1].op))
-    {
-      ops[i - 1].op = gen_rtx_MINUS (mode, ops[i - 1].op, ops[i].op);
-      ops[i - 1].op = gen_rtx_CONST (mode, ops[i - 1].op);
-      if (i < n_ops - 1)
-	ops[i] = ops[i + 1];
-      n_ops--;
-    }
-
   if (n_ops > 1
       && GET_CODE (ops[n_ops - 1].op) == CONST_INT
       && CONSTANT_P (ops[n_ops - 2].op))

============================================================================

Index: gcc/gcc/cse.c
===================================================================
--- gcc.orig/gcc/cse.c	2008-10-08 19:54:25.000000000 +0100
+++ gcc/gcc/cse.c	2008-10-08 19:55:42.000000000 +0100
@@ -3170,33 +3170,15 @@ fold_rtx (rtx x, rtx insn)
     {
     case RTX_UNARY:
       {
-	int is_const = 0;
-
 	/* We can't simplify extension ops unless we know the
 	   original mode.  */
 	if ((code == ZERO_EXTEND || code == SIGN_EXTEND)
 	    && mode_arg0 == VOIDmode)
 	  break;
 
-	/* If we had a CONST, strip it off and put it back later if we
-	   fold.  */
-	if (const_arg0 != 0 && GET_CODE (const_arg0) == CONST)
-	  is_const = 1, const_arg0 = XEXP (const_arg0, 0);
-
 	new_rtx = simplify_unary_operation (code, mode,
 					const_arg0 ? const_arg0 : folded_arg0,
 					mode_arg0);
-	/* NEG of PLUS could be converted into MINUS, but that causes
-	   expressions of the form
-	   (CONST (MINUS (CONST_INT) (SYMBOL_REF)))
-	   which many ports mistakenly treat as LEGITIMATE_CONSTANT_P.
-	   FIXME: those ports should be fixed.  */
-	if (new_rtx != 0 && is_const
-	    && GET_CODE (new_rtx) == PLUS
-	    && (GET_CODE (XEXP (new_rtx, 0)) == SYMBOL_REF
-		|| GET_CODE (XEXP (new_rtx, 0)) == LABEL_REF)
-	    && GET_CODE (XEXP (new_rtx, 1)) == CONST_INT)
-	  new_rtx = gen_rtx_CONST (mode, new_rtx);
       }
       break;
 
============================================================================

Index: gcc/gcc/doc/rtl.texi
===================================================================
--- gcc.orig/gcc/doc/rtl.texi	2008-10-08 19:53:13.000000000 +0100
+++ gcc/gcc/doc/rtl.texi	2008-10-08 20:00:03.000000000 +0100
@@ -1582,13 +1582,22 @@ Usually that is the only mode for which 
 @findex const
 @item (const:@var{m} @var{exp})
 Represents a constant that is the result of an assembly-time
-arithmetic computation.  The operand, @var{exp}, is an expression that
-contains only constants (@code{const_int}, @code{symbol_ref} and
-@code{label_ref} expressions) combined with @code{plus} and
-@code{minus}.  However, not all combinations are valid, since the
-assembler cannot do arbitrary arithmetic on relocatable symbols.
+arithmetic computation.  The valid values of @var{exp} are:
 
-@var{m} should be @code{Pmode}.
+@itemize
+@item @samp{(unspec:@var{m} @var{v} @var{c})}
+@item @samp{(plus:@var{m} (unspec:@var{m} @var{v} @var{c}) (const_int @var{offset}))}
+@item @samp{(plus:@var{m} (symbol_ref:@var{m} @var{base}) (const_int @var{offset}))}
+@item @samp{(plus:@var{m} (label_ref:@var{m} @var{base}) (const_int @var{offset}))}
+@end itemize
+
+There is no defined way for target-independent code to tell whether a
+particular @code{unspec} expression can be wrapped in a @code{const}.
+Target-independent code should therefore only create the first two
+forms if it knows that the same @samp{(unspec:@var{m} @var{v} @var{c})}
+occurs somewhere within another @code{const}'s @var{exp}.
+
+@var{m} is usually @code{Pmode}.
 
 @findex high
 @item (high:@var{m} @var{exp})