[PATCH] AArch32 target support

[Bernie Ogden]

Hi James,

Thanks for the review!

Patch 0002:

The fpu option here is for GNU tools being driven by clang. I've just done
the minimum here to give the same level of support in v8 as in v7, which
assumes fp + neon are supported. Crypto is the only extra assumption and,
given that crypto instructions won't be produced by codegen, I think this is
safe enough.

More generally, crypto is an extension to NEON (ARM-ARM section 1.6), hence
it is associated with the FPU options in other patches we have upstreamed,
as well as in the GNU tools.


Patch 0003:

To deal with the easy bits first: 

I wasn't keen on the ASCII comparison, but the really right fix is to have
something more sophisticated than a string to describe the target arch.
That's a wider change than I was trying to achieve here, so I went with the
quick fix rather than adding the dependencies and (slight) extra code
complexity to support string-to-int conversions. But I can change it if you
feel strongly about it.

NEON FPU: It seems that the v7 code only sets the predefine if the whole of
FP + NEON is available. This feels a bit odd as the neon subtarget feature
sets the vfpv2 subtarget feature - so LLVM is assuming that NEON never
exists without FP but clang (at least here) allows for the possibility of
integer-only NEON. I'm not keen on changing the v7 behaviour of this bit of
code as it seems architecturally correct. For v8, the answer is tied up with
the points about FP options below. I'd prefer to keep the possibility of
NEON-without-FP alive so long as it is cheap to do so.

So, on the optionality of FP/NEON:

1) It is legal to have non-FP implementations of v8-A for 'implementations
targeting specialised markets' (ARM-ARM section 1.5).
2) My understanding is also that NEON is not, today, optional independently
of FP.

To a point, you've caught me out just implementing the same levels of
FP/NEON/crypto functionality that exist in GCC's mfpu option for v8. I
could:

a) Leave things as they are and let this compiler choose to target FP-only,
even though the target hardware will always have NEON. Possibly emit a
warning in this case.
b) Drop the fp-armv8 option, so that we just have neon-fp-armv8 and
crypto-neon-fp-armv8
c) Make fp-armv8 mean FP + NEON

I don't like (c), just in case the architecture evolves into permitting neon
and fp to be independent in the future. They were in the past, after all,
and it is easy for us to keep them separated because that is already the way
for LLVM/clang's v7 support.

I'm open to (b) but I still prefer (a) because it gives, for free, more
flexibility for end users to impose constraints on their code and is more
GNU-compatible.

In general, I think it is sensible to allow the user to specify all sorts of
combinations for their highly specialised CPU implementation or software use
case, but to warn them when they specify something that violates the
architecture/CPU that they have targeted.

So, what do you prefer and how strongly do you feel about it?

> -----Original Message-----
> From: James Molloy
> Sent: 14 October 2013 11:58
> To: Bernard Ogden; cfe-commits at cs.uiuc.edu
> Subject: RE: [PATCH] AArch32 target support
> 
> Hi Bernie,
> 
> Patch 0001: Trivial, LGTM.
> Patch 0002: I'm not sure about crypto being enabled by default. The
> ARMARM describes this as an extension  (and one which may be disabled
> due to cryptographic/weapons export rules). Do we expect this to be so
> prevalent that it should be enabled by default?
> 
> Also, why are crypto extensions encoded into the -mfpu option? They
> don't seem to be FPU related (if that concept even exists any more).
> 
> Patch 0003: My understanding is that floating point is not an extension
> in ARMv8(A). Therefore, why do we need an FPU option for it?
> 
> +      if (CPUArch == "6T2" || CPUArch[0] >= '7')
> 
> I *really* don't like the use of the ASCII non-equality comparison
> here.
> 
> +    if ((FPU & NeonFPU) && !SoftFloat && CPUArch[0] >= '7')
> 
> Implies NeonGPU must be set for ARMv8... is this true/necessary?
> 
>    } else if (FPU == "neon-fp-armv8") {
>      Features.push_back("+fp-armv8");
>      Features.push_back("+neon");
> +    Features.push_back("-crypto");
> 
> Is NEON an extension in ArmV8? My reading of the ARMARM says that it
> isn't (the only extension mentioned by name is crypto...)
> 
> Cheers,
> 
> James
> 
> > -----Original Message-----
> > From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-
> > bounces at cs.uiuc.edu] On Behalf Of Bernie Ogden
> > Sent: 10 October 2013 18:01
> > To: cfe-commits at cs.uiuc.edu
> > Subject: [PATCH] AArch32 target support
> >
> > The attached patches-for-review build on a pair of patches I've
> submitted to
> > llvm-commits for ARM v8 AArch32 support. They tell the clang driver
> about
> > the Cortex A53 & A57 CPUs, invoke the GNU linker with the right fpu
> option
> > and deal with the various combinations of FP/NEON/Crypto support.
> >
> > I've a couple of specific queries:
> >
> > 1) I've followed a policy of supporting non-Darwin targets only. Is
> the
> > right thing to do, or should I add support for Darwin too?
> >
> > 2) There is some support for dealing with different combinations of
> > FP/NEON/Crypto features. Here I've got some plumbing (in patch 0003)
> to
> > add
> > a predefine for v8 FP, but I'm not sure what predefine to set here.
> The
> > existing code has predefines of the form __ARM_VFPV4__, which do not
> > exist
> > in GCC. I would be grateful if someone could tell me what these
> predefines
> > are for and whether I need to add another for v8 FP.
> >
> > Regards,
> >
> > Bernie