Re: [PATCH] [ARM] Cleanup macro TARGET_EITHER

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>]

Hi Richard,

On 17/03/17 13:11, Richard Earnshaw (lists) wrote:
> On 17/03/17 12:22, Kyrill Tkachov wrote:
> > Hi Sudi,
> >
> > On 16/03/17 11:11, Sudi Das wrote:
> > > Hi all
> > >
> > > This is a cleanup patch to remove the macro TARGET_EITHER. This macro
> > > seems to have become irrelevant in recent times since its previous
> > > definition had been commented out and replaced with 1.
> > >
> > > Bootstrapped and tested on arm-none-linux-gnueabihf.
> > I've had my eye on that cleanup for some time now, thanks for doing this.
> > This patch is ok.
> > Seeing as we're close to the GCC 7 release and this is not a regression fix
> > I'd rather hold off applying it until after the release, so I've put
> > this in
> > my queue of things to commit then.
> >
> > Thanks,
> > Kyrill
> >
> > > Sudi
> > >
> > > 2017-03-10  Sudakshina Das  <sudi.das@arm.com>
> > >
> > >          * config/arm/arm.h (TARGET_EITHER): Delete.
> > >          * config/arm/arm.md: Delete instances of TARGET_EITHER.
> The point of TARGET_EITHER was mostly to document that the operation was
> deliberately enabled in all compilation modes and that leaving out a
> choice of TARGET32 vs Thumb1 was not accidental.  As such, it's more
> documentation than necessary code.
>
> Does this really affect the performance/behaviour of the compiler?  If
> it doesn't then I'd be in favour of leaving it as is.

Looking at the usages of TARGET_EITHER I don't see any pattern where it looks
particularly enlightening. Some patterns gated on TARGET_EITHER have, immediately after that,
an
if (TARGET_32BIT)
 ...
else
 ...

path which makes it obvious that it handles all configurations anyway.
Some of the uses are gating fairly straightforward standard names like ashlsi3, negsi2
and saying that they are available for all configurations is not very helpful IMO as you'd
expect those fundamental patterns to be always available anyway.

Finally, TARGET_EITHER is not a particularly helpful name IMO. For a reader that would
in theory need the extra documentation, it doesn't convey what is it an EITHER *of*
(in this case 32-bit ARM/Thumb or 16-bit Thumb), they'd still need to look it up
in arm.h to see that this is what it means rather than an either of "VFP vs NEON"
or "softfloat vs hardfloat ABI" or some other binary choice in the arm backend.

Is there any particular usage of TARGET_EITHER that looks useful as documentation?
If so, I'd suggest we add a comment there clarifying it and still remove the TARGET_EITHER.

Thanks,
Kyrill

> R.