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; [email protected] > 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; [email protected] > > 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; [email protected] > > > 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: [email protected] [mailto:cfe-commits- > > > > [email protected]] On Behalf Of Bernie Ogden > > > > Sent: 10 October 2013 18:01 > > > > To: [email protected] > > > > 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 > >
0004-Add-driver-support-for-FP-SIMD-and-crypto-defaults.patch
Description: Binary data
0001-Teach-clang-driver-about-Cortex-A53-and-Cortex-A57.patch
Description: Binary data
0002-Set-appropriate-FPU-default-for-Linux-on-v8.patch
Description: Binary data
0003-Clean-up-char-numeric-comparisons-in-ARM-getTargetDe.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
