-
Notifications
You must be signed in to change notification settings - Fork 11.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
[HLSL] Cleanup TargetInfo handling #90694
[HLSL] Cleanup TargetInfo handling #90694
Conversation
We had some odd places where we set target behaviors. We were setting the long size in target-specific code, but it should be language-based. We were not setting the Half float type semantics correctly, and instead were overriding the query in the AST context. This change is NFC, but it moves existing code to the right places in the Target so that as we continue working on target and language feature they are controlled in the right places.
@llvm/pr-subscribers-hlsl @llvm/pr-subscribers-clang Author: Chris B (llvm-beanz) ChangesWe had some odd places where we set target behaviors. We were setting the long size in target-specific code, but it should be language-based. We were not setting the Half float type semantics correctly, and instead were overriding the query in the AST context. This change is NFC, but it moves existing code to the right places in the Target so that as we continue working on target and language feature they are controlled in the right places. Full diff: https://github.com/llvm/llvm-project/pull/90694.diff 3 Files Affected:
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index cbf4932aff9a6b..565870eba7014f 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -1612,15 +1612,7 @@ const llvm::fltSemantics &ASTContext::getFloatTypeSemantics(QualType T) const {
case BuiltinType::Float16:
return Target->getHalfFormat();
case BuiltinType::Half:
- // For HLSL, when the native half type is disabled, half will be treat as
- // float.
- if (getLangOpts().HLSL)
- if (getLangOpts().NativeHalfType)
- return Target->getHalfFormat();
- else
- return Target->getFloatFormat();
- else
- return Target->getHalfFormat();
+ return Target->getHalfFormat();
case BuiltinType::Float: return Target->getFloatFormat();
case BuiltinType::Double: return Target->getDoubleFormat();
case BuiltinType::Ibm128:
diff --git a/clang/lib/Basic/TargetInfo.cpp b/clang/lib/Basic/TargetInfo.cpp
index f96956f31d50dc..29f5cd14e46e11 100644
--- a/clang/lib/Basic/TargetInfo.cpp
+++ b/clang/lib/Basic/TargetInfo.cpp
@@ -406,6 +406,16 @@ void TargetInfo::adjust(DiagnosticsEngine &Diags, LangOptions &Opts) {
LongDoubleAlign = 64;
}
+ // HLSL explicitly defines the sizes and formats of some data types, and we
+ // need to conform to those regardless of what architecture you are targeting.
+ if (Opts.HLSL) {
+ LongWidth = LongAlign = 64;
+ if (!Opts.NativeHalfType) {
+ HalfFormat = &llvm::APFloat::IEEEsingle();
+ HalfWidth = HalfAlign = 32;
+ }
+ }
+
if (Opts.OpenCL) {
// OpenCL C requires specific widths for types, irrespective of
// what these normally are for the target.
diff --git a/clang/lib/Basic/Targets/DirectX.h b/clang/lib/Basic/Targets/DirectX.h
index acfcc8c47ba950..a084e2823453fc 100644
--- a/clang/lib/Basic/Targets/DirectX.h
+++ b/clang/lib/Basic/Targets/DirectX.h
@@ -53,7 +53,6 @@ class LLVM_LIBRARY_VISIBILITY DirectXTargetInfo : public TargetInfo {
: TargetInfo(Triple) {
TLSSupported = false;
VLASupported = false;
- LongWidth = LongAlign = 64;
AddrSpaceMap = &DirectXAddrSpaceMap;
UseAddrSpaceMapMangling = true;
HasLegalHalfType = true;
|
LongWidth = LongAlign = 64; | ||
if (!Opts.NativeHalfType) { | ||
HalfFormat = &llvm::APFloat::IEEEsingle(); | ||
HalfWidth = HalfAlign = 32; |
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.
Just as a note, we don't actually use this value anywhere in the HLSL code generation paths (yet), which is why it wasn't causing problems that we overrode the format returned by the AST context, but didn't override these values.
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.
Is there a test case we can add 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.
Yes, and I totally glossed over that this fixes a bug. Without this line sizeof(half)
returns the wrong value.
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.
Just a small nit from me about switch( ) statement formatting in existing code. Not the new code. Looks good to me.
We had some odd places where we set target behaviors. We were setting the long size in target-specific code, but it should be language-based. We were not setting the Half float type semantics correctly, and instead were overriding the query in the AST context.
This change it moves existing code to the right places in the Target so that as we continue working on target and language feature they are controlled in the right places.
It also fixes a bug where the size of
half
was computed incorrectly when native half types are not supported.