Re: [PATCH 1/3]middle-end: Add the ability to let the target decide the method of argument promotions.

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

Tamar Christina <Tamar.Christina@arm.com> writes:
>> -----Original Message-----
>> From: Richard Sandiford <richard.sandiford@arm.com>
>> Sent: Monday, May 16, 2022 12:36 PM
>> To: Tamar Christina <Tamar.Christina@arm.com>
>> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; rguenther@suse.de;
>> jeffreyalaw@gmail.com
>> Subject: Re: [PATCH 1/3]middle-end: Add the ability to let the target decide
>> the method of argument promotions.
>> 
>> Tamar Christina <tamar.christina@arm.com> writes:
>> > Hi All,
>> >
>> > Some targets require function parameters to be promoted to a different
>> > type on expand time because the target may not have native
>> > instructions to work on such types.  As an example the AArch64 port
>> > does not have native instructions working on integer 8- or 16-bit
>> > values.  As such it promotes every parameter of these types to 32-bits.
>> 
>> This doesn't seem specific to parameters though.  It applies to any
>> 8- or 16-bit variable.  E.g.:
>> 
>> #include <stdint.h>
>> uint8_t foo(uint32_t x, uint32_t y) {
>>     uint8_t z = x != 0 ? x : y;
>>     return z + 1;
>> }
>> 
>> generates:
>> 
>> foo:
>>         cmp     w0, 0
>>         and     w1, w1, 255
>>         and     w0, w0, 255
>>         csel    w0, w1, w0, eq
>>         add     w0, w0, 1
>>         ret
>> 
>> So I think the new behaviour is really a modification of the PROMOTE_MODE
>> behaviour rather than the PROMOTE_FUNCTION_MODE behaviour.
>> 
>> FWIW, I agree with Richard that it would be better not to add a new hook.
>> I think we're really making PROMOTE_MODE choose between
>> SIGN_EXTEND, ZERO_EXTEND or SUBREG (what LLVM would call “any
>> extend”) rather than the current SIGN_EXTEND vs. ZERO_EXTEND choice.
>
> Ah, I hadn't realized this also applied to locals.. ok I can modify PROMOTE_MODE then,
> but I also need the actual SSA_NAME and not just the type so will have to pass this along.
>
> From a practical point of view.. the actual hook however is implemented by 34 targets,
> would I need to CC maintainers for each of them or would global maintainer approval
> suffice for these mostly mechanical changes?

Yeah, single approval should be enough mechanical changes.  It would be
good to do the interface change and mechanical target changes as a
separate prepatch if possible though.

I'm not sure about passing the SSA name to the target though, or the
way that the aarch64 hook uses the info.  It looks like a single cold
comparison could defeat the optimisation for hot code.

If we do try to make the decision based on uses at expand time, it might
be better for the analysis to be in target-independent code, with help
from the target to decide where extensions are cheap.  It still feels
a bit hacky though.

What stops us from forming cbz/cbnz when the extension is done close to
the comparison (from the comment in 2/3)?  If we can solve that, could
we simply do an any-extend all the time, and treat removing redundant
extensions as a global availability problem?

What kind of code do we emit when do an extension just before
an operation?  If the DECL_RTL is (subreg:QI (reg:SI R) 0), say,
then it should be safe to do the extension directly into R:

  (set (reg:SI X) (zero_extend:SI (subreg:QI (reg:SI X))))

which avoids the problem of having two values live at once
(the zero-extended value and the any-extended value).

Having identical instructions for each extension would also make
it easier for any global pass to move them and remove redundancies.

Thanks,
Richard