[HLSL][OpenCL] Strip addrspace from implicit cast diags

[llvm-beanz]

The address space of a source value for an implicit cast isn't really relevant when emitting conversion warnings. Since the lvalue->rvalue cast effectively removes the address space they don't factor in, but they do create visual noise in the diagnostics.

This is a small quality-of-life fixup to get in as HLSL adopts more address space annotations.

[llvmbot]

@llvm/pr-subscribers-hlsl

@llvm/pr-subscribers-clang

Author: Chris B (llvm-beanz)

Changes

The address space of a source value for an implicit cast isn't really relevant when emitting conversion warnings. Since the lvalue->rvalue cast effectively removes the address space they don't factor in, but they do create visual noise in the diagnostics.

This is a small quality-of-life fixup to get in as HLSL adopts more address space annotations.

---

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

3 Files Affected:

• (modified) clang/lib/Sema/SemaChecking.cpp (+8)
• (added) clang/test/SemaHLSL/Language/ImpCastAddrSpace.hlsl (+12)
• (modified) clang/test/SemaOpenCL/cl20-device-side-enqueue.cl (+1-1)

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index d0143d29a4bcc..90293093fadd2 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -11360,6 +11360,14 @@ static void AnalyzeAssignment(Sema &S, BinaryOperator *E) {
 static void DiagnoseImpCast(Sema &S, Expr *E, QualType SourceType, QualType T,
                             SourceLocation CContext, unsigned diag,
                             bool pruneControlFlow = false) {
+  // For languages like HLSL and OpenCL, implicit conversion diagnostics listing
+  // address space annotations isn't really useful. The warnings aren't because
+  // you're converting a `private int` to `unsigned int`, it is because you're
+  // conerting `int` to `unsigned int`.
+  if (SourceType.hasAddressSpace())
+    SourceType = S.getASTContext().removeAddrSpaceQualType(SourceType);
+  if (T.hasAddressSpace())
+    T = S.getASTContext().removeAddrSpaceQualType(T);
   if (pruneControlFlow) {
     S.DiagRuntimeBehavior(E->getExprLoc(), E,
                           S.PDiag(diag)
diff --git a/clang/test/SemaHLSL/Language/ImpCastAddrSpace.hlsl b/clang/test/SemaHLSL/Language/ImpCastAddrSpace.hlsl
new file mode 100644
index 0000000000000..61e71b219b721
--- /dev/null
+++ b/clang/test/SemaHLSL/Language/ImpCastAddrSpace.hlsl
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -finclude-default-header -Wconversion -fnative-half-type %s -verify
+
+static double D = 2.0;
+static int I = D; // expected-warning{{implicit conversion turns floating-point number into integer: 'double' to 'int'}}
+groupshared float F = I; // expected-warning{{implicit conversion from 'int' to 'float' may lose precision}}
+
+export void fn() {
+  half d = I; // expected-warning{{implicit conversion from 'int' to 'half' may lose precision}}
+  int i = D; // expected-warning{{implicit conversion turns floating-point number into integer: 'double' to 'int'}}
+  int j = F; // expected-warning{{implicit conversion turns floating-point number into integer: 'float' to 'int'}}
+  int k = d; // expected-warning{{implicit conversion turns floating-point number into integer: 'half' to 'int'}}
+}
diff --git a/clang/test/SemaOpenCL/cl20-device-side-enqueue.cl b/clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
index 36b901fc5f29e..524de8ce2f7dc 100644
--- a/clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
+++ b/clang/test/SemaOpenCL/cl20-device-side-enqueue.cl
@@ -97,7 +97,7 @@ kernel void enqueue_kernel_tests(void) {
                  },
                  c, 1024L);
 #ifdef WCONV
-// expected-warning-re@-2{{implicit conversion changes signedness: '__private char' to 'unsigned {{int|long}}'}}
+// expected-warning-re@-2{{implicit conversion changes signedness: 'char' to 'unsigned {{int|long}}'}}
 #endif
 #define UINT_MAX 4294967295
 

[bob80905]

This was a helpful comment, but should these 2 if statements be nested in an if statement that checks if the language is HLSL / OpenCL?

[llvm-beanz]

No, this should apply everywhere (although it almost certainly doesn't impact C/C++ because they don't really have address spaces).

[bogner]

Should we assert here that SourceType and T aren't identical after stripping address spaces to protect against misuse of the function?

[llvm-beanz]

If we added an assert it would really be something like:

assert(!S.getASTContext().hasSameUnqualifiedType(SourceType, T) && "");

My feeling is that doesn't really add a lot of value here because so many things in Sema would have to go wrong for us to generate implicit cast AST nodes other than qualification conversions where the source and destination types have the same unqualified types.