The name mangling scheme is defined in section 3.5 of the "Vector function application binary interface specification for AArch64" [1].
Details
Diff Detail
- Repository
- rC Clang
Event Timeline
lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
9651 | Function usually are not enclosed into anonymous namespaces, mostly type declarations, enums, etc. | |
9664 |
| |
9664 | I would suggest to use QT->getCanonicalType() for checks to correctly process typedefs and aliases | |
9683 |
| |
9698 |
| |
9698 | Size must be in bits or in bytes? | |
9700 |
| |
9721 | Use llvm::SmallVector<> instead of std::vector<> | |
9722 | Remove extra parentheses around RetType->isVoidType() | |
9728 | auto->QualType | |
9742 | static function | |
9745 | auto &->const auto & | |
9757 | Extra break; | |
9777 |
| |
9791 |
| |
9821 | Remove break; | |
9827–9831 | const unsigned->unsigned, const char->char, const StringRef->StringRef | |
9832 | Do you really want to allow nullptr here? I think in this case all this stuff is useless | |
9845 | Provide SourceLocation for the report. | |
9855 | SourceLocation | |
9861 | Remove extra parens | |
9868 | SourceLocation | |
9875 | const char*->StringRef | |
9884 | else if | |
9910 |
| |
10048 | else if | |
10049–10050 | No auto here, hard to deduce types | |
10054 | else if. Or both features may exist at the same time? | |
test/OpenMP/declare-simd-fix.h | ||
1 ↗ | (On Diff #194753) | Put this file into Inputs subdirectory |
lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
9684 | Use QT.getCanonicalType() here too | |
9698 | Same here, use QT.getCanonicalType() here too | |
9715 | Use FD->getReturnType().getCanonicalType() | |
9728 | .getCanonicalType() | |
9845 | Better to use the location from the original VLEN expression. Currently it points to the function and does not help the user. | |
9855 | Same here, use the location from the VLEN | |
9861 | Parens around ISA == 's' still in here | |
9880 | Enclose into braces, if one substatement in braces, all the substatements must be in braces | |
9906 | Same, braces arounf substatement | |
10047–10057 | Enclose into braces | |
10049 | Will it work if the VLEN was not specified? |
lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
10049 | Yes, APSInt is initialized to zero. |
lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
9686 | I would suggest to checks the size of the type according to the AAVFABI and limit it with the values 1,2,4 and 8 explicitly (just in case, to handle future types) | |
9701 | There is a second part of the requirement reference to some type T for which PBV(T) is true, | |
9702 | QT.getCanonicalType()->getPointeeType() | |
9883 | I think this must be just else and in the substatement it would be good to have an assert assert(ISA == 'n' && "expected ISA either 's' or 'n'."); | |
9910 | I think this must be just else and in the substatement it would be good to have an assert assert(ISA == 'n' && "expected ISA either 's' or 'n'."); |
lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
9686 | I have added also the 16 bytes check for the complex types, and related TODO. |
Why/Where did we decide to clobber the attribute list with "non-existent function names"?
I don't think an attribute list like this:
attributes #1 = { "_ZGVsM2v_foo" "_ZGVsM32v_foo" "_ZGVsM4v_foo" "_ZGVsM6v_foo" "_ZGVsM8v_foo" "_ZGVsMxv_foo" ...
is helpful in any way, e.g., this would require us to search through all attributes and interpret them one by one.
This seems to me like an ad-hoc implementation of the RFC that is currently discussed but committed before the discussion is finished.
(I only noticed because I rebased and D62784 broke these tests)
It has nothing to do with the RFC for a variant. It is a standard interface to communicate with the backend to generate vectorized versions of the functions. It relies on Vector ABI, provided by Intel and ARM, it follows the way it is implemented in GCC. There was an RFC for this long time ago which was accepted by the community and later implemented.
The RFC states, in a nutshell, let us add one attribute to identify all vector variants. This patch adds all vector variants as attributes. Clearly, these things are related.
This new RFC just follows the scheme that was already accepted and implemented. As I understand, Francesco just wants to reuse the existing solution for SIMD isa of the pragma omp variant (or attribute clang variant)
The existence of those symbols is guaranteed by the "contract" stipulated via the Vector Function ABI. They cannot be added explicitly by the front-end as defines because they would be removed before reaching the vectorizer.
I don't think an attribute list like this:
attributes #1 = { "_ZGVsM2v_foo" "_ZGVsM32v_foo" "_ZGVsM4v_foo" "_ZGVsM6v_foo" "_ZGVsM8v_foo" "_ZGVsMxv_foo" ...
is helpful in any way, e.g., this would require us to search through all attributes and interpret them one by one.
Agree. This is what was agreed : http://lists.llvm.org/pipermail/cfe-dev/2016-March/047732.html
The new RFC will get rid of this list of string attributes. It will become something like:
attribute #0 = { declare-variant="comma,separated,list,of,vector,function,ABI,mangled,names" }.
This seems to me like an ad-hoc implementation of the RFC that is currently discussed but committed before the discussion is finished.
I can assure you that's not the case.
The code in this patch is what it is because it is based on previous (accepted) RFC originally proposed by other people and used by VecClone: https://reviews.llvm.org/D22792
As you can see in the unit tests of the VecClone pass, the variant attribute is added as follows:
attributes #0 = { nounwind uwtable "vector-variants"="_ZGVbM4_foo1,_ZGVbN4_foo1,_ZGVcM8_foo1,_ZGVcN8_foo1,_ZGVdM8_foo1,_ZGVdN8_foo1,_ZGVeM16_foo1,_ZGVeN16_foo1" .... }
Nothing in LLVM is using those attributes at the moment, that might be the reason why the string attribute have not yet been moved to a single attribute.
Kind regards,
Francesco
That is not a good argument. Afaik, there are multiple ways designed to keep symbols alive, e.g., @llvm.used.
I don't think an attribute list like this:
attributes #1 = { "_ZGVsM2v_foo" "_ZGVsM32v_foo" "_ZGVsM4v_foo" "_ZGVsM6v_foo" "_ZGVsM8v_foo" "_ZGVsMxv_foo" ...
is helpful in any way, e.g., this would require us to search through all attributes and interpret them one by one.Agree. This is what was agreed : http://lists.llvm.org/pipermail/cfe-dev/2016-March/047732.html
The new RFC will get rid of this list of string attributes. It will become something like:
attribute #0 = { declare-variant="comma,separated,list,of,vector,function,ABI,mangled,names" }.This seems to me like an ad-hoc implementation of the RFC that is currently discussed but committed before the discussion is finished.
I can assure you that's not the case.
The code in this patch is what it is because it is based on previous (accepted) RFC originally proposed by other people and used by VecClone: https://reviews.llvm.org/D22792
As you can see in the unit tests of the VecClone pass, the variant attribute is added as follows:
attributes #0 = { nounwind uwtable "vector-variants"="_ZGVbM4_foo1,_ZGVbN4_foo1,_ZGVcM8_foo1,_ZGVcN8_foo1,_ZGVdM8_foo1,_ZGVdN8_foo1,_ZGVeM16_foo1,_ZGVeN16_foo1" .... }
I get that it was discussed three years ago and I get that it was accepted then.
My confusing stems from the fact that it was committed just now, three years later, but shortly before the new RFC basically proposed a different encoding.
Nothing in LLVM is using those attributes at the moment, that might be the reason why the string attribute have not yet been moved to a single attribute.
That means we can easily change the encoding, and I strongly believe we should.
Given that you want a different encoding, I don't know if I have to list reasons why I dislike this one (direct names as attributes). Though, I can do so if we want to discuss it.
Function usually are not enclosed into anonymous namespaces, mostly type declarations, enums, etc.