Page MenuHomePhabricator

[AArch64] ARMV8-A archkind and target defines helper functions
ClosedPublic

Authored by SjoerdMeijer on Jun 27 2017, 7:20 AM.

Details

Summary

This introduces helper functions that set target defines for different ARMV8-A architecture kinds. It fixes an issue that the v8.1 define ARM_FEATURE_QRDMX was not set for v8.2. These helper functions make things more “scalable” if we want to add ARMv8.3 at some point, and a cleanup has been done to hold the architecture kind in one variable (instead of one for each).

Diff Detail

Repository
rL LLVM

Event Timeline

SjoerdMeijer created this revision.Jun 27 2017, 7:20 AM
rogfer01 edited edge metadata.Jun 28 2017, 3:13 AM

I agree with the spirit of the change but I think you want to use AArch64:: enumerators instead of those in ARM::

lib/Basic/Targets.cpp
6190 ↗(On Diff #104159)

There is an AArch64::ArchKind enum in TargetParser.h, not sure if it would be useable here.

6332 ↗(On Diff #104159)

I wonder if this is wrong, if so please fix it in a further change otherwise this one wouldn't be a NFCI.

6364 ↗(On Diff #104159)

Would it make sense to set this to AArch64::AK_ARMV8A instead?

6376 ↗(On Diff #104159)

I think you want to use llvm::AArch64::AK_ARMV8_1A instead here.

6378 ↗(On Diff #104159)

Ditto with llvm::AArch64::AK_ARMV8_2A

Thanks! I am now using llvm::AArch64::ArchKind. And I agree that the check for setting __ARM_FEATURE_QRDMX is suspicious. I will address this separately.

SjoerdMeijer retitled this revision from [AArch64] Add hasFP16VectorArithmetic helper function. NFCI to [AArch64] ARMV8-A archkind and target defines helper functions.
SjoerdMeijer edited the summary of this revision. (Show Details)

Actually, I've decided to make the functional change here because I am touching and modifying this code again.

So this is now a functional change: ARM_FEATURE_QRDMX is now defined also for ARMv8.2. This is a define for some NEON additions to ARMv8.1, which should also be included to ARMv8.2.
I've created helper functions to include defines for the different architecture kinds, and a call to include the v8.2 defines also include the v8.1 ones. Regression tests have been added for this.

I have also modified the title/summary of this ticket.

rogfer01 accepted this revision.Jun 30 2017, 12:31 AM

This LGTM. Thanks for the refactoring and the fix.

This revision is now accepted and ready to land.Jun 30 2017, 12:31 AM
This revision was automatically updated to reflect the committed changes.