Page MenuHomePhabricator

[AArch64] Implement Vector Funtion ABI name mangling.
ClosedPublic

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

Diff Detail

Repository
rC Clang

Event Timeline

fpetrogalli created this revision.Thu, Apr 11, 2:05 PM
ABataev added inline comments.Fri, Apr 12, 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
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

ABataev added inline comments.Fri, Apr 12, 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.Fri, Apr 12, 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...

10054

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

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

10047–10057

Enclose into braces

10049

Will it work if the VLEN was not specified?

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

Yes, APSInt is initialized to zero.

ABataev added inline comments.Fri, Apr 12, 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.Mon, Apr 15, 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.Mon, Apr 15, 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.Mon, Apr 15, 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.Mon, Apr 15, 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.Mon, Apr 15, 12:00 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptTue, Apr 16, 6:56 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript