Page MenuHomePhabricator

WIP Make attribute target work better with AArch64
Needs ReviewPublic

Authored by erichkeane on Sep 25 2019, 2:00 PM.

Details

Summary

This bug report: https://bugs.llvm.org/show_bug.cgi?id=43448

Highlights 2 issues with the attribute target AArch64 support.

The first is that the cpu check needs to check the arch rather than
the CPU.

The second is that the AArch64 implementation of GCC's target supports
+<FEATURE> for these targets.

This patch adds these two.

However, passing the AArch64 architecture names in target-cpu
isn't supported by LLVM:

('armv8-a' is not a recognized processor for this target (ignoring processor))

This likely needs to be fixed before this patch can go in.

Note that this is necessary for the ClangBuiltLinux effort.

Diff Detail

Event Timeline

erichkeane created this revision.Sep 25 2019, 2:00 PM
nickdesaulniers marked 2 inline comments as done.Sep 25 2019, 2:59 PM

Thanks for the patch. No comment if this is "the right thing to do," just basic code review comments.

clang/include/clang/Basic/Attr.td
2107

C++ in a .td file? *mind blown*

2126

This comment is now obsoleted by this change, I think?

2154

So many definitions of parse! If this one had Features be defaulted to getFeaturesStr(), then we could eliminate the definition on 2107.

clang/lib/Basic/Targets/AArch64.cpp
112

I think it could be a minus sign, too?

-mattr=-neon,-fp64,-d32
-mattr=+fullfp16,+thumb-mode

clang/lib/Sema/SemaDeclAttr.cpp
2976

It's too bad the TargetAttr has no notion of TargetInfo; it's kind of ugly to have to pass this in.

llvm/include/llvm/ADT/StringRef.h
783–784

So for MaxSplit and KeepEmpty I feel like YAGNI may apply here. Default arguments that are never changed in the callers is a slight code smell.

erichkeane marked 4 inline comments as done.Sep 26 2019, 6:06 AM
erichkeane added inline comments.
clang/include/clang/Basic/Attr.td
2154

This one is static.

clang/lib/Basic/Targets/AArch64.cpp
112

attribute-target doesn't support 'minus'. It is additive only.

clang/lib/Sema/SemaDeclAttr.cpp
2976

Typically we don't have attributes be context/targetinfo aware. It is a little sucky to have to pass this in, but I didn't think it too evil.

llvm/include/llvm/ADT/StringRef.h
783–784

Thats perhaps true. I was going for consistency with "split", since that is what I copied it from :)

However, passing the AArch64 architecture names in target-cpu isn't supported by LLVM

The Clang documentation suggests that arch is used to override the CPU, not the Architecture (which is rather confusing if you ask me). GCC makes more sense having separate target attributes for CPU and Architecture (see the equivalent GCC documentation). I think target-cpu should remain generic when it is not explicitly specified either on the command line (-mcpu) or as a function attribute (i.e target("arch=cortex-a57")). However, if the function attribute specifies an Architecture (i.e target("arch=armv8.4a")), I agree we should favor the subtarget features corresponding to armv8.4 over those of the command line. Similarly we should favor the subtarget features corresponding to cortex-a57 (not sure if we do so atm - I think we don't). ARM and AArch64 have a way to list the implied target features using the TargetParser but we can't directly use that in CodeGenModule because it's tied to the backend.

Ah, Ugg... GCC must implement the target parsing handling different on a per-architecture basis (ARM vs X86). We've only got parsing in place to do the x86 way. That ends up making this a much more difficult thing to maintain I suspect.

I also don't know if we have much of a way of expressing this in IR at the moment. @echristo : How much do you know of this?

ARM and AArch64 have a way to list the implied target features using the TargetParser but we can't directly use that in CodeGenModule because it's tied to the backend.

It seems that initFeatureMap allows this, but it's currently limited to inferring features from the CPU name. The ARMTargetInfo class provides an override of this function. I think for this patch to work we would need an override for AArch64 with the ability to handle Arch names.

fhahn added a comment.Oct 1 2019, 4:16 AM

Thanks for putting up the patch! Some comments inline.

clang/lib/Basic/Targets/AArch64.cpp
108

This does not seem right. The AArch64 target makes the distinction between CPU and architecture names. Unfortunately the documentation for parseCPUArch is non-existent, but it checks if the passed in CPU name matches a valid CPU name and returns the corresponding architecture of the CPU.

. I think with the change, isValidCPUName("cortex-a57") will return false, whereas it should return true. isValidCPUName("armv8-a") will return true instead of false.

I think it would be better to use parseArch directly to validate the "arch=" attribute (or add an isValidArchName here).

Also, CPU features/extensions can be validate with llvm::AArch64::parseArchExt().

clang/test/CodeGen/aarch64-target-attr.c
1

Can we also add a few negative tests, including. __attribute__((target("arch=armv8.4-a-lse"))), an invalid architecture name and an invalid CPU feature?