[PATCH] D60583: [AArch64] Implement Vector Funtion ABI name mangling.
Alexey Bataev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 12 07:44:26 PDT 2019
ABataev added inline comments.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9651
+namespace {
+// This are the Functions that are needed to mangle the name of the
----------------
Function usually are not enclosed into anonymous namespaces, mostly type declarations, enums, etc.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9664
+/// need to extend ParamKindTy to support the linear modifiers.
+bool MTV(QualType QT, ParamKindTy Kind) {
+ if (QT->isVoidType())
----------------
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
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9664
+/// need to extend ParamKindTy to support the linear modifiers.
+bool MTV(QualType QT, ParamKindTy Kind) {
+ if (QT->isVoidType())
----------------
ABataev wrote:
> 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
I would suggest to use `QT->getCanonicalType()` for checks to correctly process typedefs and aliases
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9683
+/// Pass By Value (PBV), as defined in 3.1.2 of the AAVFABI.
+bool PBV(QualType QT) {
+ if (QT->isFloatingType())
----------------
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
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9698
+/// as defined by `LS(P)` in 3.2.1 of the AAVFABI.
+unsigned LaneSize(QualType QT, ParamKindTy Kind, ASTContext &C) {
+ if (MTV(QT, Kind) && QT->isPointerType()) {
----------------
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
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9698
+/// as defined by `LS(P)` in 3.2.1 of the AAVFABI.
+unsigned LaneSize(QualType QT, ParamKindTy Kind, ASTContext &C) {
+ if (MTV(QT, Kind) && QT->isPointerType()) {
----------------
ABataev wrote:
> 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
Size must be in bits or in bytes?
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9700
+ if (MTV(QT, Kind) && QT->isPointerType()) {
+ auto PTy = cast<PointerType>(QT)->getPointeeType();
+ if (PBV(PTy))
----------------
1. `auto`->`QualType`, it is hard to deduce the type.
2. No need to do `cast<>` here
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9721
+
+ std::vector<unsigned> Sizes;
+ if (!(RetType->isVoidType())) {
----------------
Use `llvm::SmallVector<>` instead of `std::vector<>`
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9722
+ std::vector<unsigned> Sizes;
+ if (!(RetType->isVoidType())) {
+ Sizes.push_back(LaneSize(RetType, ParamKindTy::Vector, C));
----------------
Remove extra parentheses around `RetType->isVoidType()`
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9728
+ for (unsigned I = 0, E = FD->getNumParams(); I < E; ++I) {
+ auto QT = FD->getParamDecl(I)->getType();
+ Sizes.push_back(LaneSize(QT, ParamAttrs[I].Kind, C));
----------------
`auto`->`QualType`
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9742
+/// section 3.5 of the AAVFABI.
+std::string mangleVectorParameters(ArrayRef<ParamAttrTy> ParamAttrs) {
+ SmallString<256> Buffer;
----------------
static function
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9745
+ llvm::raw_svector_ostream Out(Buffer);
+ for (auto &ParamAttr : ParamAttrs) {
+ switch (ParamAttr.Kind) {
----------------
`auto &`->`const auto &`
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9757
+ break;
+ break;
+ case Uniform:
----------------
Extra `break;`
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9777
+template <typename T>
+void addAArch64VectorName(T VLEN, const char *LMask, const char *Prefix,
+ char ISA, std::string ParSeq, std::string MangledName,
----------------
1. `const char *`->`StringRef`
2. `std::string`->`StringRef`
3. make it static
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9791
+// the value of the NDS when simdlen is not present.
+void addAArch64AdvSIMDNDSNames(unsigned NDS, const char *Mask,
+ const char *Prefix, char ISA, std::string ParSeq,
----------------
1. `const char *`->`StringRef`
2. `std::string`->`StringRef`
3. make it static
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9821
+ llvm_unreachable("Scalar type is too wide.");
+ break;
+ }
----------------
Remove `break;`
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9827-9831
+ const unsigned UserVLEN,
+ ArrayRef<ParamAttrTy> ParamAttrs,
+ OMPDeclareSimdDeclAttr::BranchStateTy State,
+ const StringRef MangledName, const char ISA,
+ const unsigned VecRegSize,
----------------
`const unsigned`->`unsigned`, `const char`->`char`, `const StringRef`->`StringRef`
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9832
+ const unsigned VecRegSize,
+ llvm::Function *Fn = nullptr) {
+ // Get basic data for building the vector signature.
----------------
Do you really want to allow `nullptr` here? I think in this case all this stuff is useless
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9845
+ "The clause simdlen(1) has no effect when targeting aarch64.");
+ CGM.getDiags().Report(DiagID);
+ return;
----------------
Provide SourceLocation for the report.
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9855
+ "power of 2 when targeting Advanced SIMD.");
+ CGM.getDiags().Report(DiagID);
+ return;
----------------
SourceLocation
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9861
+ // limits.
+ if ((ISA == 's') && UserVLEN != 0) {
+ if ((UserVLEN * WDS > 2048) || (UserVLEN * WDS % 128 != 0)) {
----------------
Remove extra parens
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9868
+ "2048-bit, by steps of 128-bit)");
+ CGM.getDiags().Report(DiagID) << WDS;
+ return;
----------------
SourceLocation
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9875
+ const std::string ParSeq = mangleVectorParameters(ParamAttrs);
+ const char *Prefix = "_ZGV";
+ // Generate simdlen from user input (if any).
----------------
`const char*`->`StringRef`
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9884
+ // `[not]inbranch` clause.
+ if (ISA == 'n') {
+ switch (State) {
----------------
else if
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9910
+ // Advanced SIMD, Section 3.3.1 of the AAVFABI.
+ if ((ISA == 'n')) {
+ // Advanced SIMD generates one or two vector names depending on the use
----------------
1. Extra parens
2. else if
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:10027
emitX86DeclareSimdFunction(FD, Fn, VLENVal, ParamAttrs, State);
+ if (CGM.getTriple().getArch() == llvm::Triple::aarch64) {
+ auto VLEN = VLENVal.getExtValue();
----------------
else if
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:10028-10029
+ if (CGM.getTriple().getArch() == llvm::Triple::aarch64) {
+ auto VLEN = VLENVal.getExtValue();
+ auto MangledName = Fn->getName();
+ if (CGM.getTarget().hasFeature("sve"))
----------------
No `auto` here, hard to deduce types
================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:10033
+ MangledName, 's', 128, Fn);
+ if (CGM.getTarget().hasFeature("neon"))
+ emitAArch64DeclareSimdFunction(CGM, FD, VLEN, ParamAttrs, State,
----------------
else if. Or both features may exist at the same time?
================
Comment at: test/OpenMP/declare-simd-fix.h:1
+#pragma omp declare simd
+float foo(float a, float b, int c);
----------------
Put this file into `Inputs` subdirectory
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60583/new/
https://reviews.llvm.org/D60583
More information about the llvm-commits
mailing list