Page MenuHomePhabricator

[AArch64] Implement Vector Funtion ABI name mangling.
ClosedPublic

Authored by fpetrogalli on Apr 11 2019, 2:05 PM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ABataev added inline comments.Apr 12 2019, 7:44 AM
lib/CodeGen/CGOpenMPRuntime.cpp
9651

Function usually are not enclosed into anonymous namespaces, mostly type declarations, enums, etc.

9664
  1. Function names should be verb phrases (as they represent actions), and command-like function should be imperative. The name should be camel case, and start with a lower case letter (e.g. openFile() or isFoo()).
  2. Mark function as static
9664

I would suggest to use QT->getCanonicalType() for checks to correctly process typedefs and aliases

9683
  1. Function names should be verb phrases (as they represent actions), and command-like function should be imperative. The name should be camel case, and start with a lower case letter (e.g. openFile() or isFoo()).
  2. Mark function as static
9698
  1. Function names should be verb phrases (as they represent actions), and command-like function should be imperative. The name should be camel case, and start with a lower case letter (e.g. openFile() or isFoo()).
  2. Mark function as static
9698

Size must be in bits or in bytes?

9700
  1. auto->QualType, it is hard to deduce the type.
  2. No need to do cast<> here
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
  1. const char *->StringRef
  2. std::string->StringRef
  3. make it static
9791
  1. const char *->StringRef
  2. std::string->StringRef
  3. make it static
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
  1. Extra parens
  2. else if
10032

else if

10033–10034

No auto here, hard to deduce types

10038

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

ABataev added inline comments.Apr 12 2019, 7:47 AM
test/OpenMP/declare_simd_aarch64_fix.c
13
  1. Not all targets have /dev/null
  2. All the tests must have checks.
test/OpenMP/declare_simd_aarch64_warning_sve.c
7

Do not use FileCheck for messges checks, use -verify option

fpetrogalli marked 34 inline comments as done.Apr 12 2019, 12:03 PM

Thank you @ABataev.

lib/CodeGen/CGOpenMPRuntime.cpp
9698

I am using bytes, but it doesn't really matter, it is not required by the ABI. Do you have a preference?

9832

Yep, I have no idea why I added a nullptr default here...

10038

SVE implies AdvSIMD, so we need to generate the SIMD names too.

fpetrogalli marked 3 inline comments as done.
ABataev added inline comments.Apr 12 2019, 12:34 PM
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

10031–10041

Enclose into braces

10033

Will it work if the VLEN was not specified?

fpetrogalli marked 12 inline comments as done.
fpetrogalli added inline comments.
lib/CodeGen/CGOpenMPRuntime.cpp
10033

Yes, APSInt is initialized to zero.

ABataev added inline comments.Apr 12 2019, 1:33 PM
lib/CodeGen/CGOpenMPRuntime.cpp
9701

QT.getCanonicalType()->isPointerType()

9832

I would suggest to pass SourceLocation here rather than the Expr *. We need just a location.

fpetrogalli marked 2 inline comments as done.
fpetrogalli edited the summary of this revision. (Show Details)
ABataev added inline comments.Apr 15 2019, 8:17 AM
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'.");

fpetrogalli marked 4 inline comments as done.Apr 15 2019, 9:20 AM
fpetrogalli added inline comments.
lib/CodeGen/CGOpenMPRuntime.cpp
9686

Just for FP types? Or also for integer types?

9701

ParamKindTy doesn't support ref modifiers. Shall I add a note as a comment?

ABataev added inline comments.Apr 15 2019, 9:54 AM
lib/CodeGen/CGOpenMPRuntime.cpp
9686

This req is applied to all the types, I think

9701

YOu can still ad a checks for ref types for future fixes and add a TODO/FIXME note

fpetrogalli marked 6 inline comments as done.Apr 15 2019, 11:49 AM
fpetrogalli added inline comments.
lib/CodeGen/CGOpenMPRuntime.cpp
9686

I have added also the 16 bytes check for the complex types, and related TODO.

fpetrogalli marked an inline comment as done.
This revision is now accepted and ready to land.Apr 15 2019, 12:00 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2019, 6:56 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jdoerfert reopened this revision.EditedJun 4 2019, 1:40 PM

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)

This revision is now accepted and ready to land.Jun 4 2019, 1:40 PM

Why/Where did we decide to clobber the attribute list with "non-existent function names"?

This seems to me like an ad-hoc implementation of the RFC that is currently discussed but committed before the discussion is finished.

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.

Why/Where did we decide to clobber the attribute list with "non-existent function names"?

This seems to me like an ad-hoc implementation of the RFC that is currently discussed but committed before the discussion is finished.

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.

Why/Where did we decide to clobber the attribute list with "non-existent function names"?

This seems to me like an ad-hoc implementation of the RFC that is currently discussed but committed before the discussion is finished.

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)

fpetrogalli added a comment.EditedJun 4 2019, 2:07 PM

Why/Where did we decide to clobber the attribute list with "non-existent function names"?

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

Why/Where did we decide to clobber the attribute list with "non-existent function names"?

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.

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.

ABataev closed this revision.Jun 19 2019, 10:58 AM