This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Replace AEK_CRYPTO with relevant features in cpu definitions
ClosedPublic

Authored by dmgreen on Jan 25 2023, 8:00 AM.

Details

Summary

This replaces AEK_CRYPTO in the AArch64TargetParser definitions, replacing the composite Crypto features with the constituent parts. AEK_CRYPTO is replaced with either AEK_AES | AEK_SHA2 or AEK_AES | AEK_SHA2 | AEK_SHA3 | AEK_SHA4 depending on if the cpu is Arm-v8.4+. This helps get the features correct in some more places like target("cpu=..") attributes.

Otherwise this is hopefully an NFC for -mcpu options but seems like a cleaner design.

Diff Detail

Event Timeline

dmgreen created this revision.Jan 25 2023, 8:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2023, 8:00 AM
dmgreen requested review of this revision.Jan 25 2023, 8:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 25 2023, 8:00 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
Matt added a subscriber: Matt.Jan 25 2023, 8:53 AM
pratlucas accepted this revision.Jan 26 2023, 3:29 AM
This revision is now accepted and ready to land.Jan 26 2023, 3:29 AM
lenary accepted this revision.Jan 26 2023, 3:41 AM

Thanks, I had been thinking this was the right way to go.

I was partly wondering what we should do about FeatureCrypto in AArch64.td - the two things that came to mind were either to split it into FeatureCrypto84 and FeatureCrypto82 (names tbd), or do this full split and make FeatureCrypto do absolutely nothing (with a big comment as to why). Thoughts?

Thanks, I had been thinking this was the right way to go.

I was partly wondering what we should do about FeatureCrypto in AArch64.td - the two things that came to mind were either to split it into FeatureCrypto84 and FeatureCrypto82 (names tbd), or do this full split and make FeatureCrypto do absolutely nothing (with a big comment as to why). Thoughts?

I think the current comment for FeatureCrypto match what I had thought it should do closely enough:

// Crypto has been split up and any combination is now valid (see the
// crypto definitions above). Also, crypto is now context sensitive:
// it has a different meaning for e.g. Armv8.4 than it has for Armv8.2.
// Therefore, we rely on Clang, the user interacing tool, to pass on the
// appropriate crypto options. But here in the backend, crypto has very little
// meaning anymore. We kept the Crypto definition here for backward
// compatibility, and now imply features SHA2 and AES, which was the
// "traditional" meaning of Crypto.
def FeatureCrypto : SubtargetFeature<"crypto", "HasCrypto", "true",
  "Enable cryptographic instructions", [FeatureNEON, FeatureSHA2, FeatureAES]>;

i.e. It is there for backwards compatibility but best not to use directly. I think this works OK as-is, especially if clang stops setting it so much.

(I'm not sure that +crypto should really mean different things for 8.4, as in reality a number of cpus have come out that don't follow it, and GCC didn't implement the same thing. I don't have a very strong opinion on it though)

This revision was landed with ongoing or failed builds.Jan 30 2023, 8:05 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 8:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript