[PATCH] D60583: [AArch64] Implement Vector Funtion ABI name mangling.

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D60583#1529937 , @fpetrogalli wrote:

> In D60583#1529878 , @jdoerfert wrote:
>
> > 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 
> `define`s 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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60583/new/

https://reviews.llvm.org/D60583



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60583: [AArch64] Implement Vector Funtion ABI name mangling.

[Francesco Petrogalli via Phabricator via cfe-commits]

fpetrogalli added a comment.

In D60583#1529878 , @jdoerfert wrote:

> 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.


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


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60583/new/

https://reviews.llvm.org/D60583



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60583: [AArch64] Implement Vector Funtion ABI name mangling.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D60583#1529885 , @jdoerfert wrote:

> In D60583#1529882 , @ABataev wrote:
>
> > In D60583#1529878 , @jdoerfert 
> > wrote:
> >
> > > 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)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60583/new/

https://reviews.llvm.org/D60583



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60583: [AArch64] Implement Vector Funtion ABI name mangling.

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D60583#1529882 , @ABataev wrote:

> In D60583#1529878 , @jdoerfert wrote:
>
> > 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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60583/new/

https://reviews.llvm.org/D60583



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60583: [AArch64] Implement Vector Funtion ABI name mangling.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D60583#1529878 , @jdoerfert wrote:

> 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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60583/new/

https://reviews.llvm.org/D60583



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60583: [AArch64] Implement Vector Funtion ABI name mangling.

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert reopened this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

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.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60583/new/

https://reviews.llvm.org/D60583



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60583: [AArch64] Implement Vector Funtion ABI name mangling.

[Alexey Bataev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC358490: [AArch64] Implement Vector Funtion ABI name 
mangling. (authored by ABataev, committed by ).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60583?vs=195231=195371#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60583/new/

https://reviews.llvm.org/D60583

Files:
  lib/CodeGen/CGOpenMPRuntime.cpp
  test/OpenMP/Inputs/declare-simd-fix.h
  test/OpenMP/declare_simd_aarch64.c
  test/OpenMP/declare_simd_aarch64.cpp
  test/OpenMP/declare_simd_aarch64_complex.c
  test/OpenMP/declare_simd_aarch64_fix.c
  test/OpenMP/declare_simd_aarch64_sve.c
  test/OpenMP/declare_simd_aarch64_warning_advsimd.c
  test/OpenMP/declare_simd_aarch64_warning_sve.c

Index: lib/CodeGen/CGOpenMPRuntime.cpp
===
--- lib/CodeGen/CGOpenMPRuntime.cpp
+++ lib/CodeGen/CGOpenMPRuntime.cpp
@@ -9648,6 +9648,307 @@
   }
 }
 
+// This are the Functions that are needed to mangle the name of the
+// vector functions generated by the compiler, according to the rules
+// defined in the "Vector Function ABI specifications for AArch64",
+// available at
+// https://developer.arm.com/products/software-development-tools/hpc/arm-compiler-for-hpc/vector-function-abi.
+
+/// Maps To Vector (MTV), as defined in 3.1.1 of the AAVFABI.
+///
+/// TODO: Need to implement the behavior for reference marked with a
+/// var or no linear modifiers (1.b in the section). For this, we
+/// need to extend ParamKindTy to support the linear modifiers.
+static bool getAArch64MTV(QualType QT, ParamKindTy Kind) {
+  QT = QT.getCanonicalType();
+
+  if (QT->isVoidType())
+return false;
+
+  if (Kind == ParamKindTy::Uniform)
+return false;
+
+  if (Kind == ParamKindTy::Linear)
+return false;
+
+  // TODO: Handle linear references with modifiers
+
+  if (Kind == ParamKindTy::LinearWithVarStride)
+return false;
+
+  return true;
+}
+
+/// Pass By Value (PBV), as defined in 3.1.2 of the AAVFABI.
+static bool getAArch64PBV(QualType QT, ASTContext ) {
+  QT = QT.getCanonicalType();
+  unsigned Size = C.getTypeSize(QT);
+
+  // Only scalars and complex within 16 bytes wide set PVB to true.
+  if (Size != 8 && Size != 16 && Size != 32 && Size != 64 && Size != 128)
+return false;
+
+  if (QT->isFloatingType())
+return true;
+
+  if (QT->isIntegerType())
+return true;
+
+  if (QT->isPointerType())
+return true;
+
+  // TODO: Add support for complex types (section 3.1.2, item 2).
+
+  return false;
+}
+
+/// Computes the lane size (LS) of a return type or of an input parameter,
+/// as defined by `LS(P)` in 3.2.1 of the AAVFABI.
+/// TODO: Add support for references, section 3.2.1, item 1.
+static unsigned getAArch64LS(QualType QT, ParamKindTy Kind, ASTContext ) {
+  if (getAArch64MTV(QT, Kind) && QT.getCanonicalType()->isPointerType()) {
+QualType PTy = QT.getCanonicalType()->getPointeeType();
+if (getAArch64PBV(PTy, C))
+  return C.getTypeSize(PTy);
+  }
+  if (getAArch64PBV(QT, C))
+return C.getTypeSize(QT);
+
+  return C.getTypeSize(C.getUIntPtrType());
+}
+
+// Get Narrowest Data Size (NDS) and Widest Data Size (WDS) from the
+// signature of the scalar function, as defined in 3.2.2 of the
+// AAVFABI.
+static std::tuple
+getNDSWDS(const FunctionDecl *FD, ArrayRef ParamAttrs) {
+  QualType RetType = FD->getReturnType().getCanonicalType();
+
+  ASTContext  = FD->getASTContext();
+
+  bool OutputBecomesInput = false;
+
+  llvm::SmallVector Sizes;
+  if (!RetType->isVoidType()) {
+Sizes.push_back(getAArch64LS(RetType, ParamKindTy::Vector, C));
+if (!getAArch64PBV(RetType, C) && getAArch64MTV(RetType, {}))
+  OutputBecomesInput = true;
+  }
+  for (unsigned I = 0, E = FD->getNumParams(); I < E; ++I) {
+QualType QT = FD->getParamDecl(I)->getType().getCanonicalType();
+Sizes.push_back(getAArch64LS(QT, ParamAttrs[I].Kind, C));
+  }
+
+  assert(!Sizes.empty() && "Unable to determine NDS and WDS.");
+  // The LS of a function parameter / return value can only be a power
+  // of 2, starting from 8 bits, up to 128.
+  assert(std::all_of(Sizes.begin(), Sizes.end(),
+ [](unsigned Size) {
+   return Size == 8 || Size == 16 || Size == 32 ||
+  Size == 64 || Size == 128;
+ }) &&
+ "Invalid size");
+
+  return std::make_tuple(*std::min_element(std::begin(Sizes), std::end(Sizes)),
+ *std::max_element(std::begin(Sizes), std::end(Sizes)),
+ OutputBecomesInput);
+}
+
+/// Mangle the parameter part of the vector function name according to
+/// their OpenMP classification. The mangling function is defined in
+/// section 3.5 of the AAVFABI.
+static std::string mangleVectorParameters(ArrayRef ParamAttrs) {
+  SmallString<256>