Re: Truncate optimisation question

[Richard Sandiford <rdsandiford at googlemail dot com>]

Eric Botcazou <ebotcazou@adacore.com> writes:
>> I don't think this is the way to go.  AIUI the problem here isn't that
>> RISC architectures don't have QImode adds as such.  If we were going
>> to combine insn 6 and insn 7 _in isolation_ then we would have either:
>> 
>>    (zero_extend:SI (subreg:QI (plus:SI (subreg:QI (reg:SI R))
>>                                        (const_int X))))
>> 
>> before the patch or:
>> 
>>    (zero_extend:SI (plus:QI (reg:QI R)
>>                             (const_int X)))
>> 
>> after the patch.  And IMO the second form is a simplification over the
>> first. (Despite being a RISC port, MIPS Octeon does have a pattern for
>> zero-extending byte addition, so this isn't entirely academic.)
>
> Well, if you look at the transformation in isolation, you cannot
> reasonably say that it's a simplification for most RISC architectures
> either.  That it happens to help zero-extensions on MIPS is fine, but
> it pessimizes other cases on other architectures (and maybe on MIPS as
> well).

I think we're talking about different meanings of "simplification".
simplify-rtx.c can't and IMO shouldn't be making decisions about
simplicity in the sense of "X is cheaper on the target than Y"
or even "the target has an optab for X but not Y".  But it should
try wherever possible to (a) allow recursive simplification and
(b) reduce the number of ways that the same thing can be represented.
And the patch did that.

E.g. the old representation quoted above allowed the constant to have
redundant upper bits.  The new representation causes them to be removed,
so we have a predictable constant.

Combine is asking simplify-rtx.c to truncate an addition to QImode
and simplify-rtx.c is providing a reasonable representation of that.
It's the representation we should use when matching against .md patterns,
for example.  The problem is that combine doesn't want to keep the
truncation in this case, but doesn't know that yet.

>> It sounds like the problem is instead that we're relying on combine
>> to remove redundant zero extensions and that combine's no longer able
>> to do that when folding insns 6 and 7 into the comparison.  Is that right?
>> I think it would better to use a dedicated global pass to remove redundant
>> extensions instead.  IIRC there were various attempts to do that.
>
> The regressions affect 4.8 and mainline and it's probably too late to add new 
> passes in order to mitigate them.
>
>> IMO combine should just be about instruction selection.  The patch
>> still seems good to me from that POV.
>
> The patch is in simplify-rtx.c though, not in combine.c, so it's more general.

Right, but the only complaint I know of is about its effect on combine.
And my point is that that complaint isn't about combine failing to combine
instructions per se.  It's that combine is failing to remove a redundant
operation.  With the right input, the same rtl sequence could conceivably
be generated on a CISC target like x86_64, since it defines all the required
patterns (SImode addition, QI->SI zero extension, SImode comparison).
It could also occur with a sequence that starts out as a QImode addition.
So trying to make the simplification depend on CISCness seems like papering
over the problem.

If we want to keep this as a combine optimisation for 4.9 then I think
we should teach it to handle zero-extended arithmetic too.

But there again, I stood down as an rtl maintainer for a reason. :-)
If you think the patch was wrong or if you feel the fallout is too great
then please feel free to revert it.

Thanks,
Richard