Page MenuHomePhabricator

[ARM] Take into account -mcpu and -mfpu options while handling 'crypto' feature
ClosedPublic

Authored by krisb on Aug 9 2019, 9:14 AM.

Details

Summary

'+crypto' means '+aes' and '+sha2' for arch >= ARMv8 when they were
not disabled explicitly. But this is correctly handled only in case of
'-march' option, though the feature may also be specified through
the '-mcpu' or '-mfpu' options. In the following example:

$ clang -mcpu=cortex-a57 -mfpu=crypto-neon-fp-armv8

'aes' and 'sha2' are disabled that is quite unexpected:

$ clang -cc1 -triple armv8--- -target-cpu cortex-a57
  <...> -target-feature -sha2 -target-feature -aes -target-feature +crypto

This exposed by https://reviews.llvm.org/D63936 that makes
the 'aes' and 'sha2' features disabled by default.

So, while handling the 'crypto' feature we need to take into account:

  • a CPU name, as it provides the information about architecture (if no '-march' option specified),
  • features, specified by the '-mcpu' and '-mfpu' options.

Diff Detail

Repository
rL LLVM

Event Timeline

krisb created this revision.Aug 9 2019, 9:14 AM
dnsampaio added inline comments.Aug 14 2019, 6:28 AM
lib/Driver/ToolChains/Arch/ARM.cpp
482–486 ↗(On Diff #214387)

Could just use llvm::ARM::ArchKind arm::getLLVMArchKindForARM(StringRef CPU, StringRef Arch, const llvm::Triple &Triple) ?

krisb updated this revision to Diff 215394.Aug 15 2019, 7:16 AM

Applied the comment.

krisb added a comment.Aug 15 2019, 7:17 AM

@dnsampaio, thanks, this looks better.

Hi @krisb,
thanks for looking into this, and sorry for the delay, was out for a week.

lib/Driver/ToolChains/Arch/ARM.cpp
486–490 ↗(On Diff #215394)

Could we do something a little more permanent here, for example it is already missing ARMV8_5A.
Digging I found the function StringRef arm::getLLVMArchSuffixForARM(StringRef CPU, StringRef Arch, const llvm::Triple &Triple).
Perhaps just testing (llvm::ARM::parseArchVersion(arm::getLLVMArchSuffixForARM(StringRef CPU, StringRef Arch, const llvm::Triple &Triple)) >= 8)
would do a better job.

659 ↗(On Diff #215394)

Good catch.

krisb updated this revision to Diff 217819.Aug 29 2019, 3:29 AM

Applied the comment & rebased.
Also changed the patch to take into account only the latest 'crypto' in the Features vector, to avoid enabling 'sha2' and 'aes' when 'crypto' was finally disabled.

krisb marked 2 inline comments as done.Aug 29 2019, 3:33 AM

@dnsampaio, thanks! This is definitely better. I also added ARMV8_5A as a test case.

dnsampaio accepted this revision.Aug 29 2019, 6:09 AM

LGTM. One optional nit as it is not related with this patch anymore.

lib/Driver/ToolChains/Arch/ARM.cpp
659 ↗(On Diff #215394)

For safety perhaps we can keep the CPU.empty() test.

This revision is now accepted and ready to land.Aug 29 2019, 6:09 AM
krisb updated this revision to Diff 218076.Aug 30 2019, 6:03 AM
krisb edited the summary of this revision. (Show Details)

Added 'CPU.empty()' check back.

krisb marked an inline comment as done.Aug 30 2019, 6:06 AM

@dnsampaio thanks for reviewing this! Could I ask you to commit the patch (since I don't have commit access yet)?

dnsampaio requested changes to this revision.Aug 30 2019, 8:44 AM

clang -### -target arm-arm-none-eabit -march=armv8-m.main+crypto did not show +sha2 or +aes. After the patch it does.
I believe that is not expected, as in ARM.td crypto is not applied for any M profile. And Arm®v8-M Architecture Reference Manual does not reference these extensions neither.

lib/Driver/ToolChains/Arch/ARM.cpp
485 ↗(On Diff #218076)

&& (llvm::ARM::parseArchProfile(Arch) == llvm::ARM::ProfileKind::A)

This revision now requires changes to proceed.Aug 30 2019, 8:44 AM

I see. But this doesn't seem like a valid case, does it? Shouldn't we somehow diagnose it to not to silently ignore user-specified options?

Hi, I do agree that giving the user a warning that the argument is ignored is the best solution. If you wouldn't mind adding it to this patch, that would be great. Thanks.

krisb updated this revision to Diff 218388.Sep 2 2019, 1:24 PM
krisb marked an inline comment as done.

Applied the comment: enable 'sha2' and 'aes' only for armv8 A profile, report warning and disable 'crypto' for other archs. Add more tests.

dnsampaio accepted this revision.Sep 3 2019, 5:21 AM

LGTM. Thanks. Will commit for you as requested soon.

This revision is now accepted and ready to land.Sep 3 2019, 5:21 AM
krisb updated this revision to Diff 219670.Sep 11 2019, 1:39 AM

Rebased

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2019, 2:05 AM
krisb added a comment.Sep 11 2019, 8:18 AM

@dnsampaio, many thanks for committing this!