Alan Modra <amo...@gmail.com> writes:
> On Thu, Jan 03, 2019 at 12:41:52PM +0000, Richard Sandiford wrote:
>> Alan Modra <amo...@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
