Re: [PATCH] combine: Try harder to form zero_extends [PR106594]

[Richard Sandiford <richard.sandiford@arm.com>]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Mon, Mar 06, 2023 at 04:34:59PM +0000, Richard Sandiford wrote:
>> Jakub Jelinek <jakub@redhat.com> writes:
>> > Could we have a target hook to canonicalize memory addresses for combiner,
>> > like we have that targetm.canonicalize_comparison ?
>> 
>> I don't think a hook makes sense as a long-term design decision.
>> The canonicalisation we're doing here isn't logically AArch64-specific,
>> and in general, the less variation in RTL rules between targets, the better.
>
> Some targets do not want all insasnity allowed for other targets.  We
> have quite a few exampples of this already.  But of course a hook like
> the proposed one can be abused a lot to do completely unrelated things.
> We'll just have to trust target maintainers to have good taste and some
> wisdom (because not everyine else looks at all target patches).  What
> else is new :-)
>
>> But if you mean adding target control as a GCC 13 hack, to avoid any
>> effect on other targets, then TBH, I'd prefer just sticking it in an
>> #ifdef GCC_AARCH64_H :-)
>
> And I will NAK that for all the same reasons: it is unmaintainable, it
> makes things harder instead of solving problems, it is a completely
> ad-hoc code change.
>
>> That makes it 100% clear that it's a
>> temporary hack to restrict the target impact rather than something
>> based on fundamentals.  We can then revisit for GCC 14.
>
> And that will never happen, you know this as well as anyone else :-(
>
> Most importantly, what makes you think this is a problem for aarch64
> only?  If it actually is, you can fix it in the aarch64 config!  Either
> with or without new hooks, whatever works best.

The point is that I don't think it's a problem for AArch64 only.
I think it's a generic issue that should be solved in a generic way
(which is what the patch is trying to do).  The suggestion to restrict
it to AArch64 came from Jakub.

The reason I'm pushing back against a hook is precisely because
I don't want to solve this in AArch64-specific code.

I'm not sure we would be talking about restricting this to AArch64
if the patch had been posted in stage 1.  If people are concerned
about doing this for all targets in stage 4 (which they seem to be),
I thought the #ifdef was the simplest way of addressing that concern.
"Revisit for GCC 14" would be a case of removing the #ifdef in stage 1.

And I don't think what the patch does is ad hoc.  Reorganising the
expression in this way isn't something new.  extract_left_shift already
does a similar thing (and does it for all targets).

Thanks,
Richard