Skip to content

[clang][RISCV] Enable RVV with function attribute __attribute__((target("arch=+v"))) #83674

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 27, 2024
+85 −81
Merged

[clang][RISCV] Enable RVV with function attribute __attribute__((target("arch=+v"))) #83674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
10ab0fa
[clang][RISCV] Enable RVV with function attribute __attribute__((targ…
4vtomat Mar 1, 2024
f1d1262
fixup! [clang][RISCV] Enable RVV with function attribute __attribute_…
4vtomat Mar 4, 2024
f4d33d9
fixup! [clang][RISCV] Enable RVV with function attribute __attribute_…
4vtomat Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2234,7 +2234,8 @@ class Sema final {
bool CheckRISCVLMUL(CallExpr *TheCall, unsigned ArgNum);
bool CheckRISCVBuiltinFunctionCall(const TargetInfo &TI, unsigned BuiltinID,
CallExpr *TheCall);
void checkRVVTypeSupport(QualType Ty, SourceLocation Loc, Decl *D);
void checkRVVTypeSupport(QualType Ty, SourceLocation Loc, Decl *D,
const llvm::StringMap<bool> &FeatureMap);
bool CheckLoongArchBuiltinFunctionCall(const TargetInfo &TI,
unsigned BuiltinID, CallExpr *TheCall);
bool CheckWebAssemblyBuiltinFunctionCall(const TargetInfo &TI,
Expand Down
7 changes: 5 additions & 2 deletions clang/lib/Sema/Sema.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2065,8 +2065,11 @@ void Sema::checkTypeSupport(QualType Ty, SourceLocation Loc, ValueDecl *D) {
targetDiag(D->getLocation(), diag::note_defined_here, FD) << D;
}

if (TI.hasRISCVVTypes() && Ty->isRVVSizelessBuiltinType())
checkRVVTypeSupport(Ty, Loc, D);
if (TI.hasRISCVVTypes() && Ty->isRVVSizelessBuiltinType() && FD) {
llvm::StringMap<bool> CallerFeatureMap;
Context.getFunctionFeatureMap(CallerFeatureMap, FD);
checkRVVTypeSupport(Ty, Loc, D, CallerFeatureMap);
}

// Don't allow SVE types in functions without a SVE target.
if (Ty->isSVESizelessBuiltinType() && FD && FD->hasBody()) {
Expand Down
70 changes: 9 additions & 61 deletions clang/lib/Sema/SemaChecking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5760,57 +5760,6 @@ static bool CheckInvalidVLENandLMUL(const TargetInfo &TI, CallExpr *TheCall,
bool Sema::CheckRISCVBuiltinFunctionCall(const TargetInfo &TI,
unsigned BuiltinID,
CallExpr *TheCall) {
// CodeGenFunction can also detect this, but this gives a better error
// message.
bool FeatureMissing = false;
SmallVector<StringRef> ReqFeatures;
StringRef Features = Context.BuiltinInfo.getRequiredFeatures(BuiltinID);
Features.split(ReqFeatures, ',', -1, false);

// Check if each required feature is included
for (StringRef F : ReqFeatures) {
SmallVector<StringRef> ReqOpFeatures;
F.split(ReqOpFeatures, '|');

if (llvm::none_of(ReqOpFeatures,
[&TI](StringRef OF) { return TI.hasFeature(OF); })) {
std::string FeatureStrs;
bool IsExtension = true;
for (StringRef OF : ReqOpFeatures) {
// If the feature is 64bit, alter the string so it will print better in
// the diagnostic.
if (OF == "64bit") {
assert(ReqOpFeatures.size() == 1 && "Expected '64bit' to be alone");
OF = "RV64";
IsExtension = false;
}
if (OF == "32bit") {
assert(ReqOpFeatures.size() == 1 && "Expected '32bit' to be alone");
OF = "RV32";
IsExtension = false;
}

// Convert features like "zbr" and "experimental-zbr" to "Zbr".
OF.consume_front("experimental-");
std::string FeatureStr = OF.str();
FeatureStr[0] = std::toupper(FeatureStr[0]);
// Combine strings.
FeatureStrs += FeatureStrs.empty() ? "" : ", ";
FeatureStrs += "'";
FeatureStrs += FeatureStr;
FeatureStrs += "'";
}
// Error message
FeatureMissing = true;
Diag(TheCall->getBeginLoc(), diag::err_riscv_builtin_requires_extension)
<< IsExtension
<< TheCall->getSourceRange() << StringRef(FeatureStrs);
}
}

if (FeatureMissing)
return true;

// vmulh.vv, vmulh.vx, vmulhu.vv, vmulhu.vx, vmulhsu.vv, vmulhsu.vx,
// vsmul.vv, vsmul.vx are not included for EEW=64 in Zve64*.
switch (BuiltinID) {
Expand Down Expand Up @@ -6714,36 +6663,35 @@ bool Sema::CheckWebAssemblyBuiltinFunctionCall(const TargetInfo &TI,
return false;
}

void Sema::checkRVVTypeSupport(QualType Ty, SourceLocation Loc, Decl *D) {
const TargetInfo &TI = Context.getTargetInfo();

void Sema::checkRVVTypeSupport(QualType Ty, SourceLocation Loc, Decl *D,
const llvm::StringMap<bool> &FeatureMap) {
ASTContext::BuiltinVectorTypeInfo Info =
Context.getBuiltinVectorTypeInfo(Ty->castAs<BuiltinType>());
unsigned EltSize = Context.getTypeSize(Info.ElementType);
unsigned MinElts = Info.EC.getKnownMinValue();

if (Info.ElementType->isSpecificBuiltinType(BuiltinType::Double) &&
!TI.hasFeature("zve64d"))
!FeatureMap.lookup("zve64d"))
Diag(Loc, diag::err_riscv_type_requires_extension, D) << Ty << "zve64d";
// (ELEN, LMUL) pairs of (8, mf8), (16, mf4), (32, mf2), (64, m1) requires at
// least zve64x
else if (((EltSize == 64 && Info.ElementType->isIntegerType()) ||
MinElts == 1) &&
!TI.hasFeature("zve64x"))
!FeatureMap.lookup("zve64x"))
Diag(Loc, diag::err_riscv_type_requires_extension, D) << Ty << "zve64x";
else if (Info.ElementType->isFloat16Type() && !TI.hasFeature("zvfh") &&
!TI.hasFeature("zvfhmin"))
else if (Info.ElementType->isFloat16Type() && !FeatureMap.lookup("zvfh") &&
!FeatureMap.lookup("zvfhmin"))
Diag(Loc, diag::err_riscv_type_requires_extension, D)
<< Ty << "zvfh or zvfhmin";
else if (Info.ElementType->isBFloat16Type() &&
!TI.hasFeature("experimental-zvfbfmin"))
!FeatureMap.lookup("experimental-zvfbfmin"))
Diag(Loc, diag::err_riscv_type_requires_extension, D) << Ty << "zvfbfmin";
else if (Info.ElementType->isSpecificBuiltinType(BuiltinType::Float) &&
!TI.hasFeature("zve32f"))
!FeatureMap.lookup("zve32f"))
Diag(Loc, diag::err_riscv_type_requires_extension, D) << Ty << "zve32f";
// Given that caller already checked isRVVType() before calling this function,
// if we don't have at least zve32x supported, then we need to emit error.
else if (!TI.hasFeature("zve32x"))
else if (!FeatureMap.lookup("zve32x"))
Diag(Loc, diag::err_riscv_type_requires_extension, D) << Ty << "zve32x";
}

Expand Down
9 changes: 7 additions & 2 deletions clang/lib/Sema/SemaDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8962,8 +8962,13 @@ void Sema::CheckVariableDeclarationType(VarDecl *NewVD) {
}
}

if (T->isRVVSizelessBuiltinType())
checkRVVTypeSupport(T, NewVD->getLocation(), cast<Decl>(CurContext));
if (T->isRVVSizelessBuiltinType() && isa<FunctionDecl>(CurContext)) {
const FunctionDecl *FD = cast<FunctionDecl>(CurContext);
llvm::StringMap<bool> CallerFeatureMap;
Context.getFunctionFeatureMap(CallerFeatureMap, FD);
Copy link
Collaborator

@topperc topperc Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this impact compile time if the RVV types are used many times in a function as they would be for intrinsic code?

Copy link
Member Author

@4vtomat 4vtomat Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does.
With 10k RVV type declarations,
This pr's compile time is approximately 8 times slower than original one.(0.56 seconds vs 0.07 seconds on the server)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly costed by llvm::RISCVISAInfo::parseFeatures in RISCVTargetInfo::initFeatureMap.

Copy link
Contributor

@michaelmaitland michaelmaitland Mar 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to store a Map<FunctionDecl *, StringMap<bool> in RISCVTargetInfo class so RISCVTargetInfo::initFeatureMap only builds the feature map one time per FunctionDecl?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we don't have the FunctionDecl info in the RISCVTargetInfo::initFeatureMap call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we don't have the FunctionDecl info in the RISCVTargetInfo::initFeatureMap call.

What about keeping this map in ASTContext instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@4vtomat if the MCPU has a feature but the function explicitly disables it then I think we want the -

Why don't we just remove it from feature map?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we don't have the FunctionDecl info in the RISCVTargetInfo::initFeatureMap call.

What about keeping this map in ASTContext instead?

I guess it might be a feasible solution, however the memory usage might increase since it has to keep the map for each function during the lifetime of the ASTContext.
@topperc Do you think we can do this? Or we can leave it in TODO until it becomes a problem in some real cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@4vtomat if the MCPU has a feature but the function explicitly disables it then I think we want the -

Why don't we just remove it from feature map?

We also need the negative extensions if the target attribute string is a complete arch, e.g. __attribute__((target="arch=rv64i"))), because any extensions from mcpu need to be turned off.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukel97 Got it, thanks!

checkRVVTypeSupport(T, NewVD->getLocation(), cast<Decl>(CurContext),
CallerFeatureMap);
}
}

/// Perform semantic checking on a newly-created variable
Expand Down
22 changes: 22 additions & 0 deletions clang/test/CodeGen/RISCV/riscv-func-attr-target-err.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,28 @@
// RUN: not %clang_cc1 -triple riscv64 -target-feature +zifencei -target-feature +m -target-feature +a \
// RUN: -emit-llvm %s 2>&1 | FileCheck %s

#include <riscv_vector.h>

void test_builtin() {
// CHECK: error: '__builtin_rvv_vsetvli' needs target feature zve32x
__riscv_vsetvl_e8m8(1);
}

void test_rvv_i32_type() {
// CHECK: error: RISC-V type 'vint32m1_t' (aka '__rvv_int32m1_t') requires the 'zve32x' extension
vint32m1_t v;
}

void test_rvv_f32_type() {
// CHECK: error: RISC-V type 'vfloat32m1_t' (aka '__rvv_float32m1_t') requires the 'zve32f' extension
vfloat32m1_t v;
}

void test_rvv_f64_type() {
// CHECK: error: RISC-V type 'vfloat64m1_t' (aka '__rvv_float64m1_t') requires the 'zve64d' extension
vfloat64m1_t v;
}

// CHECK: error: duplicate 'arch=' in the 'target' attribute string;
__attribute__((target("arch=rv64gc;arch=rv64gc_zbb"))) void testMultiArchSelectLast() {}
// CHECK: error: duplicate 'cpu=' in the 'target' attribute string;
Expand Down
33 changes: 33 additions & 0 deletions clang/test/CodeGen/RISCV/riscv-func-attr-target.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
// RUN: -target-feature -relax -target-feature -zfa \
// RUN: -emit-llvm %s -o - | FileCheck %s

#include <riscv_vector.h>

// CHECK-LABEL: define dso_local void @testDefault
// CHECK-SAME: () #0 {
void testDefault() {}
Expand Down Expand Up @@ -35,6 +37,34 @@ testAttrFullArchAndAttrCpu() {}
// CHECK-SAME: () #8 {
__attribute__((target("cpu=sifive-u54"))) void testAttrCpuOnly() {}

__attribute__((target("arch=+zve32x")))
void test_builtin_w_zve32x() {
// CHECK-LABEL: test_builtin_w_zve32x
// CHECK-SAME: #9
__riscv_vsetvl_e8m8(1);
}

__attribute__((target("arch=+zve32x")))
void test_rvv_i32_type_w_zve32x() {
// CHECK-LABEL: test_rvv_i32_type_w_zve32x
// CHECK-SAME: #9
vint32m1_t v;
}

__attribute__((target("arch=+zve32f")))
void test_rvv_f32_type_w_zve32f() {
// CHECK-LABEL: test_rvv_f32_type_w_zve32f
// CHECK-SAME: #11
vfloat32m1_t v;
}

__attribute__((target("arch=+zve64d")))
void test_rvv_f64_type_w_zve64d() {
// CHECK-LABEL: test_rvv_f64_type_w_zve64d
// CHECK-SAME: #12
vfloat64m1_t v;
}

//.
// CHECK: attributes #0 = { {{.*}}"target-features"="+64bit,+a,+m,+save-restore,+zifencei,-relax,-zbb,-zfa" }
// CHECK: attributes #1 = { {{.*}}"target-cpu"="rocket-rv64" "target-features"="+64bit,+a,+d,+f,+m,+save-restore,+v,+zicsr,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl128b,+zvl32b,+zvl64b,-relax,-zbb,-zfa" "tune-cpu"="generic-rv64" }
Expand All @@ -46,3 +76,6 @@ __attribute__((target("cpu=sifive-u54"))) void testAttrCpuOnly() {}
// CHECK: attributes #6 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+a,+m,+save-restore,+zbb,+zifencei,-relax,-zfa" }
// CHECK: attributes #7 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+m,+save-restore,{{(-[[:alnum:]-]+)(,-[[:alnum:]-]+)*}}" }
// CHECK: attributes #8 = { {{.*}}"target-cpu"="sifive-u54" "target-features"="+64bit,+a,+c,+d,+f,+m,+save-restore,+zicsr,+zifencei,{{(-[[:alnum:]-]+)(,-[[:alnum:]-]+)*}}" }
// CHECK: attributes #9 = { {{.*}}"target-features"="+64bit,+a,+m,+save-restore,+zicsr,+zifencei,+zve32x,+zvl32b,-relax,-zbb,-zfa" }
// CHECK: attributes #11 = { {{.*}}"target-features"="+64bit,+a,+f,+m,+save-restore,+zicsr,+zifencei,+zve32f,+zve32x,+zvl32b,-relax,-zbb,-zfa" }
// CHECK: attributes #12 = { {{.*}}"target-features"="+64bit,+a,+d,+f,+m,+save-restore,+zicsr,+zifencei,+zve32f,+zve32x,+zve64d,+zve64f,+zve64x,+zvl32b,+zvl64b,-relax,-zbb,-zfa" }
4 changes: 2 additions & 2 deletions clang/test/CodeGen/RISCV/rvb-intrinsics/riscv32-zbb-error.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
// RUN: %clang_cc1 -triple riscv32 -target-feature +zbb -verify %s -o -
// RUN: %clang_cc1 -triple riscv32 -target-feature +zbb -S -verify %s -o -

unsigned int orc_b_64(unsigned int a) {
return __builtin_riscv_orc_b_64(a); // expected-error {{builtin requires: 'RV64'}}
return __builtin_riscv_orc_b_64(a); // expected-error {{'__builtin_riscv_orc_b_64' needs target feature zbb,64bit}}
}
12 changes: 4 additions & 8 deletions clang/test/CodeGen/RISCV/rvb-intrinsics/riscv64-zbkb-error.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
// RUN: %clang_cc1 -triple riscv64 -target-feature +zbkb -verify %s -o -
// RUN: %clang_cc1 -triple riscv64 -target-feature +zbkb -S -verify %s -o -

#include <stdint.h>

uint32_t zip(uint32_t rs1)
uint32_t zip_unzip(uint32_t rs1)
{
return __builtin_riscv_zip_32(rs1); // expected-error {{builtin requires: 'RV32'}}
}

uint32_t unzip(uint32_t rs1)
{
return __builtin_riscv_unzip_32(rs1); // expected-error {{builtin requires: 'RV32'}}
(void)__builtin_riscv_zip_32(rs1); // expected-error {{'__builtin_riscv_zip_32' needs target feature zbkb,32bit}}
return __builtin_riscv_unzip_32(rs1); // expected-error {{'__builtin_riscv_unzip_32' needs target feature zbkb,32bit}}
}
2 changes: 1 addition & 1 deletion clang/test/CodeGen/RISCV/rvv-intrinsics-handcrafted/rvv-error.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
// CHECK-RV64V-NEXT: ret i32 [[CONV]]
//

// CHECK-RV64-ERR: error: builtin requires at least one of the following extensions: 'Zve32x'
// CHECK-RV64-ERR: error: '__builtin_rvv_vsetvli' needs target feature zve32x

int test() {
return __builtin_rvv_vsetvli(1, 0, 0);
Expand Down
4 changes: 0 additions & 4 deletions clang/utils/TableGen/RISCVVEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,10 +334,6 @@ void RVVEmitter::createHeader(raw_ostream &OS) {
OS << "#include <stdint.h>\n";
OS << "#include <stddef.h>\n\n";

OS << "#ifndef __riscv_vector\n";
OS << "#error \"Vector intrinsics require the vector extension.\"\n";
OS << "#endif\n\n";

OS << "#ifdef __cplusplus\n";
OS << "extern \"C\" {\n";
OS << "#endif\n\n";
Expand Down