-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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.
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
[Clang][RISCV] bfloat uses 'y' instead of 'b' #76575
Conversation
Builtins.def says that bfloat should be represented by the 'y' character, not the 'b' character. The 'b' character is specified to use 'b'. The implementation currently uses 'b' correctly for boolean and incorrectly re-uses 'b' for bfloat. This was not caught since no builtins are emitted in build/tools/clang/include/clang/Basic/riscv_sifive_vector_builtins.inc. Don't know that we can test this without creating builtins that expose this issue, although I'm not sure we really want to do that.
@llvm/pr-subscribers-clang @llvm/pr-subscribers-backend-risc-v Author: Michael Maitland (michaelmaitland) ChangesBuiltins.def says that bfloat should be represented by the 'y' character, not the 'b' character. The 'b' character is specified to use 'b'. The implementation currently uses 'b' correctly for boolean and incorrectly re-uses 'b' for bfloat. This was not caught since no builtins are emitted in build/tools/clang/include/clang/Basic/riscv_sifive_vector_builtins.inc. Don't know that we can test this without creating builtins that expose this issue, although I'm not sure we really want to do that. Full diff: https://github.com/llvm/llvm-project/pull/76575.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/riscv_sifive_vector.td b/clang/include/clang/Basic/riscv_sifive_vector.td
index e19a34f7632fdc..0d471f6c554c22 100644
--- a/clang/include/clang/Basic/riscv_sifive_vector.td
+++ b/clang/include/clang/Basic/riscv_sifive_vector.td
@@ -109,7 +109,7 @@ multiclass RVVVFWMACCBuiltinSet<list<list<string>> suffixes_prototypes> {
Name = NAME,
HasMasked = false,
Log2LMUL = [-2, -1, 0, 1, 2] in
- defm NAME : RVVOutOp1Op2BuiltinSet<NAME, "b", suffixes_prototypes>;
+ defm NAME : RVVOutOp1Op2BuiltinSet<NAME, "y", suffixes_prototypes>;
}
multiclass RVVVQMACCDODBuiltinSet<list<list<string>> suffixes_prototypes> {
diff --git a/clang/include/clang/Basic/riscv_vector_common.td b/clang/include/clang/Basic/riscv_vector_common.td
index 4036ce8e6903f4..040db6f0cdbfb0 100644
--- a/clang/include/clang/Basic/riscv_vector_common.td
+++ b/clang/include/clang/Basic/riscv_vector_common.td
@@ -41,7 +41,7 @@
// x: float16_t (half)
// f: float32_t (float)
// d: float64_t (double)
-// b: bfloat16_t (bfloat16)
+// y: bfloat16_t (bfloat16)
//
// This way, given an LMUL, a record with a TypeRange "sil" will cause the
// definition of 3 builtins. Each type "t" in the TypeRange (in this example
diff --git a/clang/lib/Support/RISCVVIntrinsicUtils.cpp b/clang/lib/Support/RISCVVIntrinsicUtils.cpp
index bf47461b59e0ad..2de977a3dc720b 100644
--- a/clang/lib/Support/RISCVVIntrinsicUtils.cpp
+++ b/clang/lib/Support/RISCVVIntrinsicUtils.cpp
@@ -203,7 +203,7 @@ void RVVType::initBuiltinStr() {
}
break;
case ScalarTypeKind::BFloat:
- BuiltinStr += "b";
+ BuiltinStr += "y";
break;
default:
llvm_unreachable("ScalarType is invalid!");
diff --git a/clang/utils/TableGen/RISCVVEmitter.cpp b/clang/utils/TableGen/RISCVVEmitter.cpp
index da2a885ce8512f..d570bcae8d8636 100644
--- a/clang/utils/TableGen/RISCVVEmitter.cpp
+++ b/clang/utils/TableGen/RISCVVEmitter.cpp
@@ -151,7 +151,7 @@ static BasicType ParseBasicType(char c) {
case 'd':
return BasicType::Float64;
break;
- case 'b':
+ case 'y':
return BasicType::BFloat16;
break;
default:
|
Was this sentence supposed to say |
yes, updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than the comment about the commit message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than the comment about the commit message
Builtins.def says that bfloat should be represented by the 'y' character, not the 'b' character. The 'b' character is specified to represent boolean. The implementation currently uses 'b' correctly for boolean and incorrectly re-uses 'b' for bfloat.
This was not caught since no builtins are emitted in build/tools/clang/include/clang/Basic/riscv_sifive_vector_builtins.inc. Don't know that we can test this without creating builtins that expose this issue, although I'm not sure we really want to do that.