This is an archive of the discontinued LLVM Phabricator instance.

[ARM][AArch64] Require appropriate features for crypto algorithms
ClosedPublic

Authored by dcandler on Mar 22 2021, 8:01 AM.

Details

Summary

This patch changes the AArch32 crypto instructions (sha2 and aes) to
require the specific sha2 or aes features. These features have
already been implemented and can be controlled through the command
line, but do not have the expected result (i.e. +noaes will not
disable aes instructions). The crypto feature retains its existing
meaning of both sha2 and aes.

Several small changes are included due to the knock-on effect this has:

  • The AArch32 driver has been modified to ensure sha2/aes is correctly set based on arch/cpu/fpu selection and feature ordering.
  • Crypto extensions are permitted for AArch32 v8-R profile, but not enabled by default.
  • ACLE feature macros have been updated with the fine grained crypto algorithms. These are also used by AArch64.
  • Various tests updated due to the change in feature lists and macros.

Diff Detail

Event Timeline

dcandler created this revision.Mar 22 2021, 8:01 AM
dcandler requested review of this revision.Mar 22 2021, 8:01 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 22 2021, 8:01 AM
t.p.northover added inline comments.Mar 26 2021, 8:09 AM
clang/lib/Driver/ToolChains/Arch/ARM.cpp
647

Both of these checks are identical.

And could we get a comment on the purpose of this sequence? I think I can see mechanically what it's doing after a bit of thought, but still haven't come up with an example of the kind of problem it's trying to fix (not that I've tried terribly hard, that's what comments are for).

649

I think modifying Features above might have invalidated the iterator.

rsanthir.quic added inline comments.Apr 14 2021, 11:58 AM
clang/lib/Basic/Targets/AArch64.h
36

Would it make sense to further differentiate SM3 and SM4? I see that we differentiate between the two in arm_neon.td ("ARM_FEATURE_SM3" & "ARM_FEATURE_SM4") but we don't include this differentiation as flags (only HasSM4, +sm4 etc)

dcandler updated this revision to Diff 338046.Apr 16 2021, 3:16 AM

I've updated the patch to fix the test failures, and slightly reworked the driver code to avoid the above iterator invalidation. I've also added a comment there to clarify what it is doing: individually determining whether the sha2 and aes features should be enabled and explicitly setting them, since they can be controlled both by crypto and their specific feature. Using the last occurance of either in the vector ensures whatever options are passed to -mcpu/-march are evaluated in the correct order.

dcandler updated this revision to Diff 338126.Apr 16 2021, 8:26 AM
dcandler marked 2 inline comments as done.

Removed one duplicated line.

lenary accepted this revision.Apr 26 2021, 9:10 AM

LGTM. Please give the other reviewers time to chime in about this patch before merging.

This revision is now accepted and ready to land.Apr 26 2021, 9:10 AM
dcandler added inline comments.Apr 28 2021, 8:26 AM
clang/lib/Basic/Targets/AArch64.h
36

It might make more sense in the code to differentiate them, however the current feature and command line options align with GCC, so changing them would go beyond the scope of this patch.