[PATCH] AArch32 target support

[Bernie Ogden]

New patches attached - they're pretty much the same except for:

* Fixed the text/numeric comparison. There were several other instances in
the function, so I changed them all in a separate patch (0003)

* I also realised I was changing a predefine relating to thumb to apply for
v8. I've removed that change in the interest of just changing the bits
relating to what the patch is about. I expect I'll fix that predefine as
part of some thumb-targeting stuff I'm doing instead.

* I don't see the coding style violation - what am I missing?


My original two questions still apply, but if no-one answers them I'll
assume the patches are OK in these respects:

1) I'm deliberately not fiddling with Darwin-specific code - is the right
approach?

2) Can anyone tell me what the story with the FP-related predefines is? I've
got the plumbing in there to add one for v8 FP, but I'm not clear on what it
should be.

Regards,

Bernie

> -----Original Message-----
> From: James Molloy
> Sent: 14 October 2013 14:20
> To: Bernard Ogden; cfe-commits at cs.uiuc.edu
> Subject: RE: [PATCH] AArch32 target support
> 
> Hi Bernie,
> 
> OK, you had me at "GNU compatibility". I agree with (a).
> 
> Coding style nits:
> 
>   * Please do something about the textual-numeric comparison.
> StringRef::compare_numeric or StringRef::getAsInteger() are both cheap
> and available.
>   * This brace doesn't follow LLVM coding style:
> 
>      }
> +    else if (CPU == "cortex-a53" || CPU == "cortex-a57") {
> +      Features["fp-armv8"] = true;
> +      Features["neon"] = true;
> +    }
> 
> Apart from that LGTM.
> 
> Cheers,
> 
> James
> 
> > -----Original Message-----
> > From: Bernard Ogden
> > Sent: 14 October 2013 14:10
> > To: James Molloy; cfe-commits at cs.uiuc.edu
> > Subject: RE: [PATCH] AArch32 target support
> >
> > 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
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Add-driver-support-for-FP-SIMD-and-crypto-defaults.patch
Type: application/octet-stream
Size: 6240 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131018/ce6866a5/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Teach-clang-driver-about-Cortex-A53-and-Cortex-A57.patch
Type: application/octet-stream
Size: 4270 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131018/ce6866a5/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Set-appropriate-FPU-default-for-Linux-on-v8.patch
Type: application/octet-stream
Size: 1803 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131018/ce6866a5/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Clean-up-char-numeric-comparisons-in-ARM-getTargetDe.patch
Type: application/octet-stream
Size: 2563 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131018/ce6866a5/attachment-0003.obj>