genattrtab bit-rot, and if_then_else in values

This patch started off just by adding if_then_else support in
write_attr_value to be able to write a saner expression for powerpc
tls_gdld_nomark length. (*)  Then I noticed bit-rot in functions used
to calculate insn_default_length, insn_min_length, and length_unit_log
(which are used by the shorten_branches pass).  These functions
don't handle a const_int length value and return an "unknown" status
that isn't used, or in the case of or_attr_value, doesn't need to be
used.  min_attr_value also attempts to return INT_MAX for the
unhandled rtl case, but this can get lost in recursive calls.  I fixed
that problem by returning INT_MIN instead, and translating that to
INT_MAX in the only caller of min_attr_value.

Bootstrapped and regression tested powerpc64le-linux and x86_64-linux.
OK to apply?

	* genattrtab.c (max_attr_value, min_attr_value, or_attr_value):
	Delete "unknownp" parameter.  Adjust callers.  Handle
	CONST_INT and PLUS.
	(min_attr_value): Return INT_MIN for unhandled rtl case..
	(min_fn): ..and translate to INT_MAX here.
	(write_length_unit_log): Modify to cope without "unknown".
	(write_attr_value): Handle IF_THEN_ELSE.

*) I want to use something like the following in a fix for PR88614.

   (set (attr "length")
     (plus (if_then_else (match_test "TARGET_CMODEL != CMODEL_SMALL")
			 (const_int 8)
			 (const_int 4))
	   (plus (if_then_else (match_test "GET_CODE (operands[1]) != SYMBOL_REF")
			       (plus (if_then_else (match_test "!rs6000_speculate_indirect_jumps")
						   (const_int 4)
						   (const_int 0))
				     (if_then_else (match_test "DEFAULT_ABI == ABI_AIX")
						   (const_int 4)
						   (const_int 0)))
			       (const_int 0))
		 (if_then_else (match_test "DEFAULT_ABI != ABI_V4")
			       (const_int 8)
			       (const_int 4)))))

Alan Modra <amodra@gmail.com> writes:
> +    case PLUS:
> +      current_or = or_attr_value (XEXP (exp, 0));
> +      if (current_or != -1)
> +	{
> +	  int n = current_or;
> +	  current_or = or_attr_value (XEXP (exp, 1));
> +	  if (current_or != -1)
> +	    current_or += n;
> +	}
> +      break;

This doesn't look right.  Doing the same for IOR and |= would be OK
in principle, but write_attr_value doesn't handle IOR yet.

OK with the above dropped, thanks.

Richard

PS. current write_attr_value doesn't seem to handle operator precedence
properly, but that's orthogonal.

On Thu, Jan 03, 2019 at 12:41:52PM +0000, Richard Sandiford wrote:
> Alan Modra <amodra@gmail.com> writes:
> > +    case PLUS:
> > +      current_or = or_attr_value (XEXP (exp, 0));
> > +      if (current_or != -1)
> > +	{
> > +	  int n = current_or;
> > +	  current_or = or_attr_value (XEXP (exp, 1));
> > +	  if (current_or != -1)
> > +	    current_or += n;
> > +	}
> > +      break;
> 
> This doesn't look right.  Doing the same for IOR and |= would be OK
> in principle, but write_attr_value doesn't handle IOR yet.

From what I can see, or_attr_value is used to find the largest power
of two that divides all attr length values for a given insn.  So what
should be done for PLUS in an attr length value expression?  I can't
simply ignore it as then or_attr_value will return -1 and we'll
calculate length_unit_log as 0 on powerpc when it ought to be 2.
That might matter some time in the future.

If the operands of PLUS are assumed to be insn lengths (reasonable,
since you're likely to use PLUS to sum the machine insn lengths of a
pattern that conditionally emits multiple machine insns), then it
might be better to replace "current_or += n" above with
"current_or |= n".  That would be correct for a length expression of
	(plus (if_then_else (condA) (const_int 4) (const_int 0))
	      (if_then_else (condB) (const_int 8) (const_int 4)))
where any answer from or_attr_value that has 3 lsbs of 100b is
sufficently correct.  If I keep "+=" then we'd get the wrong answer in
this particular example.  However "|=" is wrong if someone is playing
games and writes a silly expression like
	(plus (const_int 1) (const_int 3))
since the length is always 4.

> OK with the above dropped, thanks.

Maybe this instead?

    case PLUS:
      current_or = or_attr_value (XEXP (exp, 0));
      current_or |= or_attr_value (XEXP (exp, 1));
      break;

> Richard
> 
> PS. current write_attr_value doesn't seem to handle operator precedence
> properly, but that's orthogonal.

I hadn't noticed that, but yes that is another problem.  I also didn't
implement MINUS, MULT, DIV and MOD in max_attr_value, min_attr_value
and or_attr_value even though write_attr_value handles those cases.

Alan Modra <amodra@gmail.com> writes:
> On Thu, Jan 03, 2019 at 12:41:52PM +0000, Richard Sandiford wrote:
>> Alan Modra <amodra@gmail.com> writes:
>> > +    case PLUS:
>> > +      current_or = or_attr_value (XEXP (exp, 0));
>> > +      if (current_or != -1)
>> > +	{
>> > +	  int n = current_or;
>> > +	  current_or = or_attr_value (XEXP (exp, 1));
>> > +	  if (current_or != -1)
>> > +	    current_or += n;
>> > +	}
>> > +      break;
>> 
>> This doesn't look right.  Doing the same for IOR and |= would be OK
>> in principle, but write_attr_value doesn't handle IOR yet.
>
> From what I can see, or_attr_value is used to find the largest power
> of two that divides all attr length values for a given insn.  So what
> should be done for PLUS in an attr length value expression?  I can't
> simply ignore it as then or_attr_value will return -1 and we'll
> calculate length_unit_log as 0 on powerpc when it ought to be 2.
> That might matter some time in the future.

Ah, OK.

> If the operands of PLUS are assumed to be insn lengths (reasonable,
> since you're likely to use PLUS to sum the machine insn lengths of a
> pattern that conditionally emits multiple machine insns), then it
> might be better to replace "current_or += n" above with
> "current_or |= n".  That would be correct for a length expression of
> 	(plus (if_then_else (condA) (const_int 4) (const_int 0))
> 	      (if_then_else (condB) (const_int 8) (const_int 4)))
> where any answer from or_attr_value that has 3 lsbs of 100b is
> sufficently correct.  If I keep "+=" then we'd get the wrong answer in
> this particular example.  However "|=" is wrong if someone is playing
> games and writes a silly expression like
> 	(plus (const_int 1) (const_int 3))
> since the length is always 4.
>
>> OK with the above dropped, thanks.
>
> Maybe this instead?
>
>     case PLUS:
>       current_or = or_attr_value (XEXP (exp, 0));
>       current_or |= or_attr_value (XEXP (exp, 1));
>       break;

This still seems risky and isn't what the name and function comment
imply.  Maybe we should replace or_attr_value with something like
floor_log2_attr_value or attr_value_alignment?

Thanks,
Richard

Richard Sandiford <richard.sandiford@arm.com> writes:
> Alan Modra <amodra@gmail.com> writes:
>> If the operands of PLUS are assumed to be insn lengths (reasonable,
>> since you're likely to use PLUS to sum the machine insn lengths of a
>> pattern that conditionally emits multiple machine insns), then it
>> might be better to replace "current_or += n" above with
>> "current_or |= n".  That would be correct for a length expression of
>> 	(plus (if_then_else (condA) (const_int 4) (const_int 0))
>> 	      (if_then_else (condB) (const_int 8) (const_int 4)))
>> where any answer from or_attr_value that has 3 lsbs of 100b is
>> sufficently correct.  If I keep "+=" then we'd get the wrong answer in
>> this particular example.  However "|=" is wrong if someone is playing
>> games and writes a silly expression like
>> 	(plus (const_int 1) (const_int 3))
>> since the length is always 4.
>>
>>> OK with the above dropped, thanks.
>>
>> Maybe this instead?
>>
>>     case PLUS:
>>       current_or = or_attr_value (XEXP (exp, 0));
>>       current_or |= or_attr_value (XEXP (exp, 1));
>>       break;
>
> This still seems risky and isn't what the name and function comment
> imply.  Maybe we should replace or_attr_value with something like
> floor_log2_attr_value or attr_value_alignment?

Er, forget floor_log2_attr_value :-)  First day back, etc. etc.

On Thu, Jan 03, 2019 at 07:03:59PM +0000, Richard Sandiford wrote:
> Richard Sandiford <richard.sandiford@arm.com> writes:
> > This still seems risky and isn't what the name and function comment

OK, how about this delta from the previous patch to ameliorate the
maintenance risk?

diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c
index c86b3587cd9..2a81445a160 100644
--- a/gcc/genattrtab.c
+++ b/gcc/genattrtab.c
@@ -3750,8 +3750,9 @@ write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int flags,
   return attrs_cached;
 }
 
-/* Given an attribute value, return the maximum CONST_INT or CONST_STRING
-   argument encountered.  Return INT_MAX if the value is unknown.  */
+/* Given an attribute value expression, return the maximum value that
+   might be evaluated assuming all conditionals are independent.
+   Return INT_MAX if the value can't be calculated by this function.  */
 
 static int
 max_attr_value (rtx exp)
@@ -3805,8 +3806,9 @@ max_attr_value (rtx exp)
   return current_max;
 }
 
-/* Given an attribute value, return the minimum CONST_INT or CONST_STRING
-   argument encountered.  Return INT_MIN if the value is unknown.  */
+/* Given an attribute value expression, return the minimum value that
+   might be evaluated assuming all conditionals are independent.
+   Return INT_MIN if the value can't be calculated by this function.  */
 
 static int
 min_attr_value (rtx exp)
@@ -3860,9 +3862,18 @@ min_attr_value (rtx exp)
   return current_min;
 }
 
-/* Given an attribute value, return the result of ORing together all
-   CONST_INT or CONST_STRING arguments encountered.  Return -1 if the
-   numeric value is not known.  */
+/* Given an attribute value expression, return the result of ORing
+   together all CONST_INT or CONST_STRING arguments encountered in the
+   value parts of the expression.  (ie. excluding those that might be
+   found in condition expressions of COND or IF_THEN_ELSE.)  Note that
+   this isn't the same as the OR of all possible evaluations of the
+   expression, but is sufficient for the current use of this function
+   in calculating the largest power of two that divides insn lengths.
+   ie.  We are calculating a property of the underlying machine
+   instructions emitted by gcc.  In particular this implementation
+   assumes conditionals are independent and that the operands of a
+   PLUS are multiples of the underlying machine instruction length.
+   Returns -1 if the value can't be calculated by this function.  */
 
 static int
 or_attr_value (rtx exp)
@@ -3882,13 +3893,7 @@ or_attr_value (rtx exp)
 
     case PLUS:
       current_or = or_attr_value (XEXP (exp, 0));
-      if (current_or != -1)
-	{
-	  int n = current_or;
-	  current_or = or_attr_value (XEXP (exp, 1));
-	  if (current_or != -1)
-	    current_or += n;
-	}
+      current_or |= or_attr_value (XEXP (exp, 1));
       break;
 
     case COND:

Alan Modra <amodra@gmail.com> writes:
> On Thu, Jan 03, 2019 at 07:03:59PM +0000, Richard Sandiford wrote:
>> Richard Sandiford <richard.sandiford@arm.com> writes:
>> > This still seems risky and isn't what the name and function comment
>
> OK, how about this delta from the previous patch to ameliorate the
> maintenance risk?

attr_value_alignment seems clearer, and means that we can handle
things like:

  (mult (symbol_ref "...") (const_int 4))

How about the patch below?

Thanks,
Richard


2019-01-04  Richard Sandiford  <richard.sandiford@arm.com>

gcc/
	* genattrtab.c (or_attr_value): Replace with...
	(attr_value_alignment): ...this new function.  Handle PLUS,
	MINUS and MULT.
	(write_length_unit_log): Update accordingly.

Index: gcc/genattrtab.c
===================================================================
--- gcc/genattrtab.c	2019-01-04 12:16:51.000000000 +0000
+++ gcc/genattrtab.c	2019-01-04 12:16:51.322900008 +0000
@@ -268,7 +268,7 @@ static void insert_insn_ent        (stru
 static void walk_attr_value	   (rtx);
 static int max_attr_value	   (rtx, int*);
 static int min_attr_value	   (rtx, int*);
-static int or_attr_value	   (rtx, int*);
+static unsigned int attr_value_alignment (rtx);
 static rtx simplify_test_exp	   (rtx, int, int);
 static rtx simplify_test_exp_in_temp (rtx, int, int);
 static rtx copy_rtx_unchanging	   (rtx);
@@ -1568,24 +1568,21 @@ write_length_unit_log (FILE *outf)
   struct attr_value *av;
   struct insn_ent *ie;
   unsigned int length_unit_log, length_or;
-  int unknown = 0;
 
   if (length_attr)
     {
-      length_or = or_attr_value (length_attr->default_val->value, &unknown);
+      length_or = attr_value_alignment (length_attr->default_val->value);
       for (av = length_attr->first_value; av; av = av->next)
 	for (ie = av->first_insn; ie; ie = ie->next)
-	  length_or |= or_attr_value (av->value, &unknown);
-    }
+	  length_or |= attr_value_alignment (av->value);
 
-  if (length_attr == NULL || unknown)
-    length_unit_log = 0;
-  else
-    {
       length_or = ~length_or;
       for (length_unit_log = 0; length_or & 1; length_or >>= 1)
 	length_unit_log++;
     }
+  else
+    length_unit_log = 0;
+
   fprintf (outf, "EXPORTED_CONST int length_unit_log = %u;\n", length_unit_log);
 }
 
@@ -3835,16 +3832,16 @@ min_attr_value (rtx exp, int *unknownp)
   return current_min;
 }
 
-/* Given an attribute value, return the result of ORing together all
-   CONST_STRING arguments encountered.  Set *UNKNOWNP and return -1
-   if the numeric value is not known.  */
+/* Given an attribute value EXP, return the maximum alignment that all
+   values are known to have.  Return 0 if EXP is known to be zero.  */
 
-static int
-or_attr_value (rtx exp, int *unknownp)
+static unsigned int
+attr_value_alignment (rtx exp)
 {
-  int current_or;
+  unsigned int current_or;
   int i;
 
+  /* When breaking, OR the possible alignment values into CURRENT_OR.  */
   switch (GET_CODE (exp))
     {
     case CONST_STRING:
@@ -3852,23 +3849,31 @@ or_attr_value (rtx exp, int *unknownp)
       break;
 
     case COND:
-      current_or = or_attr_value (XEXP (exp, 1), unknownp);
+      current_or = attr_value_alignment (XEXP (exp, 1));
       for (i = 0; i < XVECLEN (exp, 0); i += 2)
-	current_or |= or_attr_value (XVECEXP (exp, 0, i + 1), unknownp);
+	current_or |= attr_value_alignment (XVECEXP (exp, 0, i + 1));
       break;
 
     case IF_THEN_ELSE:
-      current_or = or_attr_value (XEXP (exp, 1), unknownp);
-      current_or |= or_attr_value (XEXP (exp, 2), unknownp);
+      current_or = attr_value_alignment (XEXP (exp, 1));
+      current_or |= attr_value_alignment (XEXP (exp, 2));
       break;
 
-    default:
-      *unknownp = 1;
-      current_or = -1;
+    case PLUS:
+    case MINUS:
+      current_or = attr_value_alignment (XEXP (exp, 0));
+      current_or |= attr_value_alignment (XEXP (exp, 1));
       break;
+
+    case MULT:
+      return (attr_value_alignment (XEXP (exp, 0))
+	      * attr_value_alignment (XEXP (exp, 1)));
+
+    default:
+      return 1;
     }
 
-  return current_or;
+  return current_or & -current_or;
 }
 
 /* Scan an attribute value, possibly a conditional, and record what actions

On Fri, Jan 04, 2019 at 12:18:03PM +0000, Richard Sandiford wrote:
> Alan Modra <amodra@gmail.com> writes:
> > On Thu, Jan 03, 2019 at 07:03:59PM +0000, Richard Sandiford wrote:
> >> Richard Sandiford <richard.sandiford@arm.com> writes:
> >> > This still seems risky and isn't what the name and function comment
> >
> > OK, how about this delta from the previous patch to ameliorate the
> > maintenance risk?
> 
> attr_value_alignment seems clearer, and means that we can handle
> things like:
> 
>   (mult (symbol_ref "...") (const_int 4))

OK, revised patch as follows, handling MINUS and MULT in the max/min
value functions too.

	* genattrtab.c (max_attr_value, min_attr_value, or_attr_value):
	Delete "unknownp" parameter.  Adjust callers.  Handle
	CONST_INT, PLUS, MINUS, and MULT.
	(attr_value_aligned): Renamed from or_attr_value.
	(min_attr_value): Return INT_MIN for unhandled rtl case..
	(min_fn): ..and translate to INT_MAX here.
	(write_length_unit_log): Modify to cope without "unknown".
	(write_attr_value): Handle IF_THEN_ELSE.

diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c
index 2cd04cdb06f..b8adf704009 100644
--- a/gcc/genattrtab.c
+++ b/gcc/genattrtab.c
@@ -266,9 +266,9 @@ static int compares_alternatives_p (rtx);
 static void make_internal_attr     (const char *, rtx, int);
 static void insert_insn_ent        (struct attr_value *, struct insn_ent *);
 static void walk_attr_value	   (rtx);
-static int max_attr_value	   (rtx, int*);
-static int min_attr_value	   (rtx, int*);
-static int or_attr_value	   (rtx, int*);
+static int max_attr_value	   (rtx);
+static int min_attr_value	   (rtx);
+static unsigned int attr_value_alignment (rtx);
 static rtx simplify_test_exp	   (rtx, int, int);
 static rtx simplify_test_exp_in_temp (rtx, int, int);
 static rtx copy_rtx_unchanging	   (rtx);
@@ -1550,15 +1550,16 @@ one_fn (rtx exp ATTRIBUTE_UNUSED)
 static rtx
 max_fn (rtx exp)
 {
-  int unknown;
-  return make_numeric_value (max_attr_value (exp, &unknown));
+  return make_numeric_value (max_attr_value (exp));
 }
 
 static rtx
 min_fn (rtx exp)
 {
-  int unknown;
-  return make_numeric_value (min_attr_value (exp, &unknown));
+  int val = min_attr_value (exp);
+  if (val < 0)
+    val = INT_MAX;
+  return make_numeric_value (val);
 }
 
 static void
@@ -1568,24 +1569,21 @@ write_length_unit_log (FILE *outf)
   struct attr_value *av;
   struct insn_ent *ie;
   unsigned int length_unit_log, length_or;
-  int unknown = 0;
 
   if (length_attr)
     {
-      length_or = or_attr_value (length_attr->default_val->value, &unknown);
+      length_or = attr_value_alignment (length_attr->default_val->value);
       for (av = length_attr->first_value; av; av = av->next)
 	for (ie = av->first_insn; ie; ie = ie->next)
-	  length_or |= or_attr_value (av->value, &unknown);
-    }
+	  length_or |= attr_value_alignment (av->value);
 
-  if (length_attr == NULL || unknown)
-    length_unit_log = 0;
-  else
-    {
       length_or = ~length_or;
       for (length_unit_log = 0; length_or & 1; length_or >>= 1)
 	length_unit_log++;
     }
+  else
+    length_unit_log = 0;
+
   fprintf (outf, "EXPORTED_CONST int length_unit_log = %u;\n", length_unit_log);
 }
 
@@ -3753,11 +3751,12 @@ write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int flags,
   return attrs_cached;
 }
 
-/* Given an attribute value, return the maximum CONST_STRING argument
-   encountered.  Set *UNKNOWNP and return INT_MAX if the value is unknown.  */
+/* Given an attribute value expression, return the maximum value that
+   might be evaluated assuming all conditionals are independent.
+   Return INT_MAX if the value can't be calculated by this function.  */
 
 static int
-max_attr_value (rtx exp, int *unknownp)
+max_attr_value (rtx exp)
 {
   int current_max;
   int i, n;
@@ -3768,25 +3767,62 @@ max_attr_value (rtx exp, int *unknownp)
       current_max = atoi (XSTR (exp, 0));
       break;
 
+    case CONST_INT:
+      current_max = INTVAL (exp);
+      break;
+
+    case PLUS:
+      current_max = max_attr_value (XEXP (exp, 0));
+      if (current_max != INT_MAX)
+	{
+	  n = current_max;
+	  current_max = max_attr_value (XEXP (exp, 1));
+	  if (current_max != INT_MAX)
+	    current_max += n;
+	}
+      break;
+
+    case MINUS:
+      current_max = max_attr_value (XEXP (exp, 0));
+      if (current_max != INT_MAX)
+	{
+	  n = min_attr_value (XEXP (exp, 1));
+	  if (n == INT_MIN)
+	    current_max = INT_MAX;
+	  else
+	    current_max -= n;
+	}
+      break;
+
+    case MULT:
+      current_max = max_attr_value (XEXP (exp, 0));
+      if (current_max != INT_MAX)
+	{
+	  n = current_max;
+	  current_max = max_attr_value (XEXP (exp, 1));
+	  if (current_max != INT_MAX)
+	    current_max *= n;
+	}
+      break;
+
     case COND:
-      current_max = max_attr_value (XEXP (exp, 1), unknownp);
+      current_max = max_attr_value (XEXP (exp, 1));
       for (i = 0; i < XVECLEN (exp, 0); i += 2)
 	{
-	  n = max_attr_value (XVECEXP (exp, 0, i + 1), unknownp);
+	  n = max_attr_value (XVECEXP (exp, 0, i + 1));
 	  if (n > current_max)
 	    current_max = n;
 	}
       break;
 
     case IF_THEN_ELSE:
-      current_max = max_attr_value (XEXP (exp, 1), unknownp);
-      n = max_attr_value (XEXP (exp, 2), unknownp);
+      current_max = max_attr_value (XEXP (exp, 1));
+      n = max_attr_value (XEXP (exp, 2));
       if (n > current_max)
 	current_max = n;
       break;
 
     default:
-      *unknownp = 1;
       current_max = INT_MAX;
       break;
     }
@@ -3794,11 +3830,12 @@ max_attr_value (rtx exp, int *unknownp)
   return current_max;
 }
 
-/* Given an attribute value, return the minimum CONST_STRING argument
-   encountered.  Set *UNKNOWNP and return 0 if the value is unknown.  */
+/* Given an attribute value expression, return the minimum value that
+   might be evaluated assuming all conditionals are independent.
+   Return INT_MIN if the value can't be calculated by this function.  */
 
 static int
-min_attr_value (rtx exp, int *unknownp)
+min_attr_value (rtx exp)
 {
   int current_min;
   int i, n;
@@ -3809,40 +3846,77 @@ min_attr_value (rtx exp, int *unknownp)
       current_min = atoi (XSTR (exp, 0));
       break;
 
+    case CONST_INT:
+      current_min = INTVAL (exp);
+      break;
+
+    case PLUS:
+      current_min = min_attr_value (XEXP (exp, 0));
+      if (current_min != INT_MIN)
+	{
+	  n = current_min;
+	  current_min = min_attr_value (XEXP (exp, 1));
+	  if (current_min != INT_MIN)
+	    current_min += n;
+	}
+      break;
+
+    case MINUS:
+      current_min = min_attr_value (XEXP (exp, 0));
+      if (current_min != INT_MIN)
+	{
+	  n = max_attr_value (XEXP (exp, 1));
+	  if (n == INT_MAX)
+	    current_min = INT_MIN;
+	  else
+	    current_min -= n;
+	}
+      break;
+
+    case MULT:
+      current_min = min_attr_value (XEXP (exp, 0));
+      if (current_min != INT_MIN)
+	{
+	  n = current_min;
+	  current_min = min_attr_value (XEXP (exp, 1));
+	  if (current_min != INT_MIN)
+	    current_min *= n;
+	}
+      break;
+
     case COND:
-      current_min = min_attr_value (XEXP (exp, 1), unknownp);
+      current_min = min_attr_value (XEXP (exp, 1));
       for (i = 0; i < XVECLEN (exp, 0); i += 2)
 	{
-	  n = min_attr_value (XVECEXP (exp, 0, i + 1), unknownp);
+	  n = min_attr_value (XVECEXP (exp, 0, i + 1));
 	  if (n < current_min)
 	    current_min = n;
 	}
       break;
 
     case IF_THEN_ELSE:
-      current_min = min_attr_value (XEXP (exp, 1), unknownp);
-      n = min_attr_value (XEXP (exp, 2), unknownp);
+      current_min = min_attr_value (XEXP (exp, 1));
+      n = min_attr_value (XEXP (exp, 2));
       if (n < current_min)
 	current_min = n;
       break;
 
     default:
-      *unknownp = 1;
-      current_min = INT_MAX;
+      current_min = INT_MIN;
       break;
     }
 
   return current_min;
 }
 
-/* Given an attribute value, return the result of ORing together all
-   CONST_STRING arguments encountered.  Set *UNKNOWNP and return -1
-   if the numeric value is not known.  */
+/* Given an attribute value expression, return the alignment of values
+   assuming conditionals are independent.  Returns 0 if EXP is known to
+   be zero, and 1 if the value can't be calculated by this function.  */
 
-static int
-or_attr_value (rtx exp, int *unknownp)
+static unsigned int
+attr_value_alignment (rtx exp)
 {
-  int current_or;
+  unsigned int current_or;
   int i;
 
   switch (GET_CODE (exp))
@@ -3851,24 +3925,38 @@ or_attr_value (rtx exp, int *unknownp)
       current_or = atoi (XSTR (exp, 0));
       break;
 
+    case CONST_INT:
+      current_or = INTVAL (exp);
+      break;
+
+    case PLUS:
+    case MINUS:
+      current_or = attr_value_alignment (XEXP (exp, 0));
+      current_or |= attr_value_alignment (XEXP (exp, 1));
+      break;
+
+    case MULT:
+      current_or = attr_value_alignment (XEXP (exp, 0));
+      current_or *= attr_value_alignment (XEXP (exp, 1));
+      break;
+
     case COND:
-      current_or = or_attr_value (XEXP (exp, 1), unknownp);
+      current_or = attr_value_alignment (XEXP (exp, 1));
       for (i = 0; i < XVECLEN (exp, 0); i += 2)
-	current_or |= or_attr_value (XVECEXP (exp, 0, i + 1), unknownp);
+	current_or |= attr_value_alignment (XVECEXP (exp, 0, i + 1));
       break;
 
     case IF_THEN_ELSE:
-      current_or = or_attr_value (XEXP (exp, 1), unknownp);
-      current_or |= or_attr_value (XEXP (exp, 2), unknownp);
+      current_or = attr_value_alignment (XEXP (exp, 1));
+      current_or |= attr_value_alignment (XEXP (exp, 2));
       break;
 
     default:
-      *unknownp = 1;
-      current_or = -1;
+      current_or = 1;
       break;
     }
 
-  return current_or;
+  return current_or & -current_or;
 }
 
 /* Scan an attribute value, possibly a conditional, and record what actions
@@ -4343,6 +4431,16 @@ write_attr_value (FILE *outf, struct attr_desc *attr, rtx value)
       write_attr_value (outf, attr, XEXP (value, 1));
       break;
 
+    case IF_THEN_ELSE:
+      fprintf (outf, "(");
+      write_test_expr (outf, XEXP (value, 0), 0, 0, false);
+      fprintf (outf, " ? ");
+      write_attr_value (outf, attr, XEXP (value, 1));
+      fprintf (outf, " : ");
+      write_attr_value (outf, attr, XEXP (value, 2));
+      fprintf (outf, ")");
+      break;
+
     default:
       gcc_unreachable ();
     }

Alan Modra <amodra@gmail.com> writes:
> On Fri, Jan 04, 2019 at 12:18:03PM +0000, Richard Sandiford wrote:
>> Alan Modra <amodra@gmail.com> writes:
>> > On Thu, Jan 03, 2019 at 07:03:59PM +0000, Richard Sandiford wrote:
>> >> Richard Sandiford <richard.sandiford@arm.com> writes:
>> >> > This still seems risky and isn't what the name and function comment
>> >
>> > OK, how about this delta from the previous patch to ameliorate the
>> > maintenance risk?
>> 
>> attr_value_alignment seems clearer, and means that we can handle
>> things like:
>> 
>>   (mult (symbol_ref "...") (const_int 4))
>
> OK, revised patch as follows, handling MINUS and MULT in the max/min
> value functions too.
>
> 	* genattrtab.c (max_attr_value, min_attr_value, or_attr_value):
> 	Delete "unknownp" parameter.  Adjust callers.  Handle
> 	CONST_INT, PLUS, MINUS, and MULT.
> 	(attr_value_aligned): Renamed from or_attr_value.
> 	(min_attr_value): Return INT_MIN for unhandled rtl case..
> 	(min_fn): ..and translate to INT_MAX here.
> 	(write_length_unit_log): Modify to cope without "unknown".
> 	(write_attr_value): Handle IF_THEN_ELSE.
>
> diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c
> index 2cd04cdb06f..b8adf704009 100644
> --- a/gcc/genattrtab.c
> +++ b/gcc/genattrtab.c
> @@ -266,9 +266,9 @@ static int compares_alternatives_p (rtx);
>  static void make_internal_attr     (const char *, rtx, int);
>  static void insert_insn_ent        (struct attr_value *, struct insn_ent *);
>  static void walk_attr_value	   (rtx);
> -static int max_attr_value	   (rtx, int*);
> -static int min_attr_value	   (rtx, int*);
> -static int or_attr_value	   (rtx, int*);
> +static int max_attr_value	   (rtx);
> +static int min_attr_value	   (rtx);
> +static unsigned int attr_value_alignment (rtx);
>  static rtx simplify_test_exp	   (rtx, int, int);
>  static rtx simplify_test_exp_in_temp (rtx, int, int);
>  static rtx copy_rtx_unchanging	   (rtx);
> @@ -1550,15 +1550,16 @@ one_fn (rtx exp ATTRIBUTE_UNUSED)
>  static rtx
>  max_fn (rtx exp)
>  {
> -  int unknown;
> -  return make_numeric_value (max_attr_value (exp, &unknown));
> +  return make_numeric_value (max_attr_value (exp));
>  }
>  
>  static rtx
>  min_fn (rtx exp)
>  {
> -  int unknown;
> -  return make_numeric_value (min_attr_value (exp, &unknown));
> +  int val = min_attr_value (exp);
> +  if (val < 0)
> +    val = INT_MAX;
> +  return make_numeric_value (val);
>  }
>  
>  static void
> @@ -1568,24 +1569,21 @@ write_length_unit_log (FILE *outf)
>    struct attr_value *av;
>    struct insn_ent *ie;
>    unsigned int length_unit_log, length_or;
> -  int unknown = 0;
>  
>    if (length_attr)
>      {
> -      length_or = or_attr_value (length_attr->default_val->value, &unknown);
> +      length_or = attr_value_alignment (length_attr->default_val->value);
>        for (av = length_attr->first_value; av; av = av->next)
>  	for (ie = av->first_insn; ie; ie = ie->next)
> -	  length_or |= or_attr_value (av->value, &unknown);
> -    }
> +	  length_or |= attr_value_alignment (av->value);
>  
> -  if (length_attr == NULL || unknown)
> -    length_unit_log = 0;
> -  else
> -    {
>        length_or = ~length_or;
>        for (length_unit_log = 0; length_or & 1; length_or >>= 1)
>  	length_unit_log++;
>      }
> +  else
> +    length_unit_log = 0;
> +
>    fprintf (outf, "EXPORTED_CONST int length_unit_log = %u;\n", length_unit_log);
>  }
>  
> @@ -3753,11 +3751,12 @@ write_test_expr (FILE *outf, rtx exp, unsigned int attrs_cached, int flags,
>    return attrs_cached;
>  }
>  
> -/* Given an attribute value, return the maximum CONST_STRING argument
> -   encountered.  Set *UNKNOWNP and return INT_MAX if the value is unknown.  */
> +/* Given an attribute value expression, return the maximum value that
> +   might be evaluated assuming all conditionals are independent.
> +   Return INT_MAX if the value can't be calculated by this function.  */

Not sure about "assuming all conditionals are independent".  All three
functions should be conservatively correct without any assumptions.

OK without that part if you agree.

Thanks,
Richard

On Mon, Jan 07, 2019 at 12:10:38PM +0000, Richard Sandiford wrote:
> Alan Modra <amodra@gmail.com> writes:
> > +/* Given an attribute value expression, return the maximum value that
> > +   might be evaluated assuming all conditionals are independent.
> > +   Return INT_MAX if the value can't be calculated by this function.  */
> 
> Not sure about "assuming all conditionals are independent".  All three
> functions should be conservatively correct without any assumptions.

True.  I'll drop that phrase.  The functions aren't even guaranteed to
give an exactly correct result if they contain any conditions..

> OK without that part if you agree.