-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[lldb][TypeSystemClang] Added unique builtins types for __bf16 and _Float16 #157674
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
[lldb][TypeSystemClang] Added unique builtins types for __bf16 and _Float16 #157674
Conversation
@llvm/pr-subscribers-lldb Author: None (tgs-sc) ChangesDuring debugging applization with __bf16 and _Float16 float types it was discovered that lldb creates the same CompilerType for them. This can cause an infinite recursion error, if one tries to create two struct specializations with these types and then inherit one specialization from another. This is an example, provided by @Michael137: template <typename T> struct Foo;
template <> struct Foo<__bf16> {};
template <> struct Foo<_Float16> : Foo<__bf16> {};
int main() {
Foo<_Float16> f1;
return 0;
} Full diff: https://github.com/llvm/llvm-project/pull/157674.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index c4a917f59fb88..804ddd042574e 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -959,6 +959,12 @@ CompilerType TypeSystemClang::GetBuiltinTypeForDWARFEncodingAndBitSize(
if (type_name == "long double" &&
QualTypeMatchesBitSize(bit_size, ast, ast.LongDoubleTy))
return GetType(ast.LongDoubleTy);
+ if (type_name == "__bf16" &&
+ QualTypeMatchesBitSize(bit_size, ast, ast.BFloat16Ty))
+ return GetType(ast.BFloat16Ty);
+ if (type_name == "_Float16" &&
+ QualTypeMatchesBitSize(bit_size, ast, ast.Float16Ty))
+ return GetType(ast.Float16Ty);
// As Rust currently uses `TypeSystemClang`, match `f128` here as well so it
// doesn't get misinterpreted as `long double` on targets where they are
// the same size but different formats.
@@ -1791,6 +1797,7 @@ bool TypeSystemClang::RecordHasFields(const RecordDecl *record_decl) {
for (base_class = cxx_record_decl->bases_begin(),
base_class_end = cxx_record_decl->bases_end();
base_class != base_class_end; ++base_class) {
+ assert(record_decl != base_class->getType()->getAsCXXRecordDecl());
if (RecordHasFields(base_class->getType()->getAsCXXRecordDecl()))
return true;
}
diff --git a/lldb/test/API/lang/cpp/template-arguments/TestCppTemplateArguments.py b/lldb/test/API/lang/cpp/template-arguments/TestCppTemplateArguments.py
index eac7b5ef1099a..f26d382bf8582 100644
--- a/lldb/test/API/lang/cpp/template-arguments/TestCppTemplateArguments.py
+++ b/lldb/test/API/lang/cpp/template-arguments/TestCppTemplateArguments.py
@@ -82,7 +82,7 @@ def test(self):
value = self.expect_expr("temp7", result_type="Foo<__fp16, __fp16>")
self.assertFalse(value.GetType().GetTemplateArgumentValue(target, 1))
- value = self.expect_expr("temp8", result_type="Foo<__fp16, __fp16>")
+ value = self.expect_expr("temp8", result_type="Foo<__bf16, __bf16>")
self.assertFalse(value.GetType().GetTemplateArgumentValue(target, 1))
value = self.expect_expr("temp9", result_type="Bar<double, 1.200000e+00>")
|
9ee07b2
to
a18616f
Compare
@Michael137, Hi, I have added here a test you proposed. Can you please look at this patch as it is blocking merging PR(#154123). |
@JDevlieghere, hi! Can you also please look at this patch? I think it is pretty obvious. |
@Michael137, Hi! Sorry for disturbing you. A week has passed, can you please look at this patch? |
@@ -1791,6 +1797,7 @@ bool TypeSystemClang::RecordHasFields(const RecordDecl *record_decl) { | |||
for (base_class = cxx_record_decl->bases_begin(), | |||
base_class_end = cxx_record_decl->bases_end(); | |||
base_class != base_class_end; ++base_class) { | |||
assert(record_decl != base_class->getType()->getAsCXXRecordDecl()); |
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.
assert(record_decl != base_class->getType()->getAsCXXRecordDecl()); | |
assert(record_decl != base_class->getType()->getAsCXXRecordDecl() && "Base can't inherit from itself."); |
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.
Addressed
@@ -82,7 +82,7 @@ def test(self): | |||
value = self.expect_expr("temp7", result_type="Foo<__fp16, __fp16>") |
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.
Shouldn't this one show up as _Float16
now? Or is there some other place we're missing support for this?
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.
There is a mapping in clang/include/clang/AST/BuiltinTypes.def
that maps Float16 to HalfTy, so actually it shouldn't be displayed as _Float16.
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.
I can make such change:
clang/include/clang/AST/BuiltinTypes.def
// '_Float16'
FLOATING_TYPE(Float16, HalfTy)
->
clang/include/clang/AST/BuiltinTypes.def
// '_Float16'
FLOATING_TYPE(Float16, Float16Ty)
But I am not sure that it won't break anything. Probably we should ask someone on the clang side why this is made in such way.
…loat16 During debugging applization with __bf16 and _Float16 float types it was discovered that lldb creates the same CompilerType for them. This can cause an infinite recursion error, if one tries to create two struct specializations with these types and then inherit one specialization from another.
a18616f
to
7461016
Compare
@Michael137, can you please look at this? |
@Michael137, can you please then merge this patch? Because I don't have corresponding rights. |
This is failing on Arm 32-bit: https://lab.llvm.org/buildbot/#/builders/18/builds/21624
AArch64 is fine - https://lab.llvm.org/buildbot/#/builders/59/builds/25028 Gonna look at the test case a bit more to see why. |
A few printfs later:
ARM has a bf16 type in the AST context only if this is true:
And for whatever reason, the lookup is set to a target without bf16 or an FPU. Not sure where we pull the target setting from (istr some triple to core details mapping) but regardless, we're missing FPU (which my host does have) and bfloat16 (which my host does not have). For AArch64 this method just returns true. I'll find the source of the target type but in the meantime I'll skip or work around this failure. |
Fixes #157674 On ARM, the presence of a specific bf16 type in the AST is gated by: ``` bool ARMTargetInfo::hasBFloat16Type() const { // The __bf16 type is generally available so long as we have any fp registers. return HasBFloat16 || (FPU && !SoftFloat); } ``` And the target we use when evaluating symbols (derived from the program file, I think, haven't found it yet) does not enable any of this. This means that we fall back to __fp16. So for parts of the testing we just need to expect __fp16 instead, and others we need to skip because now a class looks like it inherits from itself. There's a proper fix here somewhere but I don't know what it is yet.
…loat16 (llvm#157674) During debugging applization with __bf16 and _Float16 float types it was discovered that lldb creates the same CompilerType for them. This can cause an infinite recursion error, if one tries to create two struct specializations with these types and then inherit one specialization from another.
Fixes llvm#157674 On ARM, the presence of a specific bf16 type in the AST is gated by: ``` bool ARMTargetInfo::hasBFloat16Type() const { // The __bf16 type is generally available so long as we have any fp registers. return HasBFloat16 || (FPU && !SoftFloat); } ``` And the target we use when evaluating symbols (derived from the program file, I think, haven't found it yet) does not enable any of this. This means that we fall back to __fp16. So for parts of the testing we just need to expect __fp16 instead, and others we need to skip because now a class looks like it inherits from itself. There's a proper fix here somewhere but I don't know what it is yet.
During debugging applization with __bf16 and _Float16 float types it was discovered that lldb creates the same CompilerType for them. This can cause an infinite recursion error, if one tries to create two struct specializations with these types and then inherit one specialization from another.