Skip to content
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][CUDA] Disable float128 diagnostics for device compilation #83918

Merged
merged 3 commits into from
Mar 7, 2024

Conversation

pranavk
Copy link
Contributor

@pranavk pranavk commented Mar 4, 2024

No description provided.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 4, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 4, 2024

@llvm/pr-subscribers-clang

Author: Pranav Kant (pranavk)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/83918.diff

2 Files Affected:

  • (modified) clang/lib/Sema/SemaDeclAttr.cpp (+3-1)
  • (modified) clang/lib/Sema/SemaType.cpp (+1)
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 397b5db0dc0669..e6943efb345ce0 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -4877,7 +4877,9 @@ void Sema::AddModeAttr(Decl *D, const AttributeCommonInfo &CI,
     NewElemTy = Context.getRealTypeForBitwidth(DestWidth, ExplicitType);
 
   if (NewElemTy.isNull()) {
-    Diag(AttrLoc, diag::err_machine_mode) << 1 /*Unsupported*/ << Name;
+    // Only emit diagnostic on host for 128-bit mode attribute
+    if (!(DestWidth == 128 && getLangOpts().CUDAIsDevice))
+      Diag(AttrLoc, diag::err_machine_mode) << 1 /*Unsupported*/ << Name;
     return;
   }
 
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 1e43e36016a66f..4a4e6f80d0d049 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -1562,6 +1562,7 @@ static QualType ConvertDeclSpecToType(TypeProcessingState &state) {
   case DeclSpec::TST_float128:
     if (!S.Context.getTargetInfo().hasFloat128Type() &&
         !S.getLangOpts().SYCLIsDevice &&
+        !S.getLangOpts().CUDAIsDevice &&
         !(S.getLangOpts().OpenMP && S.getLangOpts().OpenMPIsTargetDevice))
       S.Diag(DS.getTypeSpecTypeLoc(), diag::err_type_unsupported)
         << "__float128";

Copy link

github-actions bot commented Mar 4, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@@ -4877,7 +4877,9 @@ void Sema::AddModeAttr(Decl *D, const AttributeCommonInfo &CI,
NewElemTy = Context.getRealTypeForBitwidth(DestWidth, ExplicitType);

if (NewElemTy.isNull()) {
Diag(AttrLoc, diag::err_machine_mode) << 1 /*Unsupported*/ << Name;
// Only emit diagnostic on host for 128-bit mode attribute
Copy link
Member

Choose a reason for hiding this comment

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

What do you expect to see if __float128 is used from a GPU function.

Can you check on a toy example.

__attribute__((device)) __float128 f(__float128 a, float b) {
  __float128 c = b + 1.0;
  return a + c;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is going to error out like this:

error: 'a' requires 128 bit size '__float128' type support, but target 'nvptx64-nvidia-cuda' does not support it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, it may be a good idea to add that in the diagnostic test.

Copy link
Member

Choose a reason for hiding this comment

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

This is going to error out like this:

error: 'a' requires 128 bit size '__float128' type support, but target 'nvptx64-nvidia-cuda' does not support it

Something does not add up. How would we get target 'nvptx64-nvidia-cuda' if the diag below only fires if we're compiling for the host?

Copy link
Contributor Author

@pranavk pranavk Mar 5, 2024

Choose a reason for hiding this comment

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

There are two diags that I disabled as part of this PR:

  1. %0 is not supported on this target
  2. unsupported machine mode __TC__

The diag that got emitted as part of building sample snippet that you pasted in this comment thread is neither 1 nor 2. This diag comes from part of clang/LLVM when it starts to generate code for nvptx specifically.

Copy link
Member

Choose a reason for hiding this comment

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

OK. As long as you're sure that the remaining diag covers all possible uses of fp128 on the GPU, it should be fine.

// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -aux-triple nvptx64 -x cuda -fsyntax-only -verify %s

// GPU-side compilation on x86 (no errors expected)
// RUN: %clang_cc1 -triple nvptx64 -aux-triple x86_64-unknown-linux-gnu -fcuda-is-device -x cuda -fsyntax-only -verify %s
Copy link
Member

Choose a reason for hiding this comment

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

I'd add a test verifying that we do emit diagnostics if fp128 is used in the GPU code.
It would probably need to be done somewhere in the codegen tests as it will not fire in the syntax-only checks.

@pranavk pranavk merged commit 318bff6 into llvm:main Mar 7, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants