[PATCH] genattrtab bit-rot, and if_then_else in values

[Richard Sandiford]

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