Skip to content

Emit a warning for floating-point size changes after implicit conversions #6323

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.

Sign up for GitHub

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

Merged
merged 34 commits into from
Aug 16, 2022
Merged

Emit a warning for floating-point size changes after implicit conversions #6323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
c570dad
[SYCL] Emit a warning if double precision arithmetic is used in devic…
srividya-sundaram Jun 17, 2022
289bbaa
Add warning to warning-flags.c
srividya-sundaram Jun 17, 2022
be0b892
Fix warnings
srividya-sundaram Jun 25, 2022
dc02117
Add test cases for builtins and default arg promo
srividya-sundaram Jun 27, 2022
5754157
Add new line at the end of file
srividya-sundaram Jun 27, 2022
0bcd3f4
Fix failing tests
srividya-sundaram Jun 30, 2022
762d50a
Fix failing tests
srividya-sundaram Jul 1, 2022
3217c76
Fix warnings-flag.c
srividya-sundaram Jul 1, 2022
374e9a2
Address review comments.
srividya-sundaram Jul 5, 2022
35ee34c
Fix review comments
srividya-sundaram Jul 5, 2022
6041fb9
WIP - Experimental changes.
srividya-sundaram Jul 13, 2022
c3b98d3
fix lint
srividya-sundaram Jul 13, 2022
50f903b
fix tests
srividya-sundaram Jul 13, 2022
5dec41a
fix test
srividya-sundaram Jul 13, 2022
e2c2358
Merge branch 'sycl' into warn-double-precision
srividya-sundaram Jul 13, 2022
c40235f
lint fix
srividya-sundaram Jul 13, 2022
1d978d6
Merge branch 'warn-double-precision' of https://github.com/srividya-s…
srividya-sundaram Jul 13, 2022
459aa08
Ignore warning by default
srividya-sundaram Jul 14, 2022
43c632a
Fix test
srividya-sundaram Jul 14, 2022
568512f
fix test
srividya-sundaram Jul 14, 2022
67c3190
Rename file
srividya-sundaram Jul 14, 2022
12a5286
test changes
srividya-sundaram Jul 17, 2022
1dad8e8
Update logic
srividya-sundaram Jul 18, 2022
7780205
Fix review comments
srividya-sundaram Jul 18, 2022
d6cad3c
Fix tests
srividya-sundaram Jul 18, 2022
78445c3
Update test
srividya-sundaram Jul 19, 2022
23e73be
Add new test to check conversion warnings
srividya-sundaram Jul 29, 2022
41c2560
Update clang/include/clang/Basic/DiagnosticSemaKinds.td
srividya-sundaram Aug 1, 2022
1e00605
Update clang/lib/Sema/SemaChecking.cpp
srividya-sundaram Aug 1, 2022
c09926f
Update clang/lib/Sema/SemaChecking.cpp
srividya-sundaram Aug 2, 2022
192099e
Add custom prefixes
srividya-sundaram Aug 5, 2022
6e2964c
Address review comments
srividya-sundaram Aug 5, 2022
96b9aac
Fix test
srividya-sundaram Aug 8, 2022
b2f59ce
Fix extra space
srividya-sundaram Aug 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion clang/include/clang/Basic/DiagnosticGroups.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,11 @@ def ImplicitIntFloatConversion : DiagGroup<"implicit-int-float-conversion",
[ImplicitConstIntFloatConversion]>;
def ObjCSignedCharBoolImplicitFloatConversion :
DiagGroup<"objc-signed-char-bool-implicit-float-conversion">;
def ImplicitFloatSizeConversion :
DiagGroup<"implicit-float-size-conversion">;
def ImplicitFloatConversion : DiagGroup<"implicit-float-conversion",
[ImplicitIntFloatConversion,
ObjCSignedCharBoolImplicitFloatConversion]>;
ObjCSignedCharBoolImplicitFloatConversion, ImplicitFloatSizeConversion]>;
def ImplicitFixedPointConversion : DiagGroup<"implicit-fixed-point-conversion">;

def FloatOverflowConversion : DiagGroup<"float-overflow-conversion">;
Expand Down
3 changes: 3 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,9 @@ def warn_float_compare_literal : Warning<
def warn_double_const_requires_fp64 : Warning<
"double precision constant requires %select{cl_khr_fp64|cl_khr_fp64 and __opencl_c_fp64}0, "
"casting to single precision">;
def warn_imp_float_size_conversion : Warning<
"implicit conversion between floating point types of different sizes">,
InGroup<ImplicitFloatSizeConversion>, DefaultIgnore;
def err_half_const_requires_fp16 : Error<
"half precision constant requires cl_khr_fp16">;

Expand Down
24 changes: 20 additions & 4 deletions clang/lib/Sema/SemaChecking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13815,15 +13815,31 @@ static void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
Expr::EvalResult result;
if (E->EvaluateAsRValue(result, S.Context)) {
// Value might be a float, a float vector, or a float complex.
if (IsSameFloatAfterCast(result.Val,
S.Context.getFloatTypeSemantics(QualType(TargetBT, 0)),
S.Context.getFloatTypeSemantics(QualType(SourceBT, 0))))
if (IsSameFloatAfterCast(
result.Val,
S.Context.getFloatTypeSemantics(QualType(TargetBT, 0)),
S.Context.getFloatTypeSemantics(QualType(SourceBT, 0)))) {
if (S.getLangOpts().SYCLIsDevice)
S.SYCLDiagIfDeviceCode(CC, diag::warn_imp_float_size_conversion);
else
DiagnoseImpCast(S, E, T, CC,
diag::warn_imp_float_size_conversion);
return;
}
}

if (S.SourceMgr.isInSystemMacro(CC))
return;

// If there is a precision conversion between floating point types when
// -Wimplicit-float-size-conversion is passed but
// -Wimplicit-float-conversion is not, make sure we emit at least a size
// warning.
if (S.Diags.isIgnored(diag::warn_impcast_float_precision, CC)) {
if (S.getLangOpts().SYCLIsDevice)
S.SYCLDiagIfDeviceCode(CC, diag::warn_imp_float_size_conversion);
else
DiagnoseImpCast(S, E, T, CC, diag::warn_imp_float_size_conversion);
}
DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_float_precision);
}
// ... or possibly if we're increasing rank, too
Expand Down
4 changes: 2 additions & 2 deletions clang/test/Sema/conversion.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ void test13(long double v) {
takes_int(v); // expected-warning {{implicit conversion turns floating-point number into integer}}
takes_long(v); // expected-warning {{implicit conversion turns floating-point number into integer}}
takes_longlong(v); // expected-warning {{implicit conversion turns floating-point number into integer}}
takes_float(v); // expected-warning {{implicit conversion loses floating-point precision}}
takes_float(v); // expected-warning {{implicit conversion loses floating-point precision}}
takes_double(v); // expected-warning {{implicit conversion loses floating-point precision}}
takes_longdouble(v);
}
Expand Down Expand Up @@ -430,7 +430,6 @@ void test27(ushort16 constants) {
ushort16 brBias = pairedConstants.s6; // expected-warning {{implicit conversion loses integer precision: 'uint32_t' (aka 'unsigned int') to 'ushort16'}}
}


float double2float_test1(double a) {
return a; // expected-warning {{implicit conversion loses floating-point precision: 'double' to 'float'}}
}
Expand All @@ -448,3 +447,4 @@ float double2float_test4(double a, float b) {
b -= a; // expected-warning {{implicit conversion when assigning computation result loses floating-point precision: 'double' to 'float'}}
return b;
}
float f = 1.0 / 2.0; // expected-warning {{implicit conversion between floating point types of different sizes}}
2 changes: 1 addition & 1 deletion clang/test/Sema/ext_vector_casts.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ static void splats(int i, long l, __uint128_t t, float f, double d) {

vf = 1 + vf;
vf = l + vf; // expected-warning {{implicit conversion from 'long' to 'float2' (vector of 2 'float' values) may lose precision}}
vf = 2.0 + vf;
vf = 2.0 + vf; // expected-warning {{implicit conversion between floating point types of different sizes}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also add a test for vd like: vd = 2.0f + vd; (we should get the same diagnostic about different sizes because of the addition between different types).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vd is a vector of 2 double values. Since this is a conversion from float to double there is no precision loss and a warning is not triggered here. Also, if the vector cast is cast between two vectors of the same size, it is a bitcast, not a conversion - https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaChecking.cpp#L13796

Copy link
Contributor

Choose a reason for hiding this comment

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

vd is a vector of 2 double values. Since this is a conversion from float to double there is no precision loss and a warning is not triggered here.

The new diagnostic is about size conversions, not precision changes.

Also, if the vector cast is cast between two vectors of the same size, it is a bitcast, not a conversion - https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SemaChecking.cpp#L13796

If we're diagnosing this as being a size conversion:

  vf = 2.0 + vf; // expected-warning {{implicit conversion between floating point types of different sizes}}

what is your rationale for not diagnosing this similarly:

vd = 2.0f + vd;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After debugging, it turns out that this is a case of double promotion.
If we pass the flag -Wdouble-promotion, we will get the following warning:

implicit conversion increases floating-point precision: 'float' to 'double2' (vector of 2 'double' values)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh god. I had no idea we had a diagnostic like that! UGH. So it sounds like there's yet another diagnostic in this same general space that we need to think about (it's a GCC diagnostic Clang added for compatibility, as best I can tell). This covers some of what the user was asking for, but not all of it, because it only handles an increase in precision not a change in size.

(I hate the diagnostic group it's under because this is not a promotion (default argument promotions are what happen when you call a variadic function and pass a float, as in: void func(int a, ...); void foo(void) { func(1, 1.0f); } the 1.0f is promoted to a double in that case).)

I think we need to think about how these diagnostics interact a bit more carefully. It's not clear to me which diagnostic to prefer in what circumstance yet, and there's significant overlap between the size conversion warning you're working on and the existing -Wdouble-promotion warning, it seems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this test added as requested above:
vd = 2.0f + vd;

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 test case has been included in clang/test/Sema/precision-and-size-conversion.c

Copy link
Contributor

Choose a reason for hiding this comment

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

This test case has been included in clang/test/Sema/precision-and-size-conversion.c

Sorry, I missed this test, thanks. Does the diagnostic get issued on the device? Did that get addressed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The double promotion diagnostic is only available on host. Enabling it on device wasn't part of the scope of this change set.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that that wasn't part of the original ask of this PR, but given what we discovered along the way, I feel it completes what might be useful for the user on device. @AaronBallman, what do you feel about this?

Can you explain what you mean by "only available on host"? Is the codepath somehow disabled on target? Thanks.

vf = d + vf; // expected-warning {{implicit conversion loses floating-point precision}}
vf = vf + 0xffffffff; // expected-warning {{implicit conversion from 'unsigned int' to 'float2' (vector of 2 'float' values) changes value from 4294967295 to 4294967296}}
vf = vf + 2.1; // expected-warning {{implicit conversion loses floating-point precision}}
Expand Down
26 changes: 26 additions & 0 deletions clang/test/Sema/precision-and-size-conversion.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// RUN: %clang_cc1 -fsyntax-only -verify=precision-loss,precision-gain,size-change -Wimplicit-float-conversion -Wdouble-promotion -Wimplicit-float-size-conversion \
// RUN: -triple x86_64-apple-darwin %s
Copy link
Contributor

Choose a reason for hiding this comment

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

For my own understanding, what is the significance of this triple? Why are we using this instead of say, x86_64-pc-windows-msvc that we typically see on sycl host?

Copy link
Contributor Author

@srividya-sundaram srividya-sundaram Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

There is no significance behind chosing the darwin target. It could as well be windows-msvc and the test would pass.
I just picked it up from some tests I referenced and felt adventurous to go with it!


// RUN: %clang_cc1 -fsyntax-only -verify=size-only,precision-gain,size-change -Wdouble-promotion -Wimplicit-float-size-conversion \
// RUN: -triple x86_64-apple-darwin %s

// RUN: %clang_cc1 -fsyntax-only -verify=precision-increase -Wdouble-promotion \
// RUN: -triple x86_64-apple-darwin %s

// RUN: %clang_cc1 -fsyntax-only -verify=precision-loss,size-change -Wimplicit-float-conversion \
// RUN: -triple x86_64-apple-darwin %s

// RUN: %clang_cc1 -fsyntax-only -verify \
// RUN: -triple x86_64-apple-darwin %s

// This test checks that floating point conversion warnings are emitted correctly when used in conjunction.

// expected-no-diagnostics
// size-only-warning@+2 {{implicit conversion between floating point types of different sizes}}
// precision-loss-warning@+1 {{implicit conversion loses floating-point precision: 'double' to 'float'}}
float PrecisionLoss = 1.1;
// precision-increase-warning@+2 {{implicit conversion increases floating-point precision: 'float' to 'double'}}
// precision-gain-warning@+1 {{implicit conversion increases floating-point precision: 'float' to 'double'}}
double PrecisionIncrease = 2.1f;
// size-change-warning@+1 {{implicit conversion between floating point types of different sizes}}
float SizeChange = 3.0;
26 changes: 26 additions & 0 deletions clang/test/SemaSYCL/implicit-float-size-conversion.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// RUN: %clang_cc1 -fsycl-is-device -triple spir64 -internal-isystem %S/Inputs -fsyntax-only -sycl-std=2020 -Wimplicit-float-size-conversion -verify=size-only,always-size %s
// RUN: %clang_cc1 -fsycl-is-device -triple spir64 -internal-isystem %S/Inputs -fsyntax-only -sycl-std=2020 -Wimplicit-float-conversion -verify=always-size,precision-only %s
// RUN: %clang_cc1 -fsycl-is-device -triple spir64 -internal-isystem %S/Inputs -fsyntax-only -sycl-std=2020 -Wimplicit-float-conversion -Wno-implicit-float-size-conversion -verify=prefer-precision %s
// RUN: %clang_cc1 -fsycl-is-device -triple spir64 -internal-isystem %S/Inputs -fsyntax-only -sycl-std=2020 -Wno-implicit-float-conversion -verify %s

// This test checks that floating point conversion warnings are emitted correctly when used in conjunction.

#include "sycl.hpp"
class kernelA;

using namespace cl::sycl;

int main() {
queue q;
// expected-no-diagnostics
// always-size-note@#KernelSingleTaskKernelFuncCall {{called by 'kernel_single_task<kernelA, (lambda}}
q.submit([&](handler &h) {
h.single_task<class kernelA>([=]() {
float s = 1.0; // always-size-warning {{implicit conversion between floating point types of different sizes}}
// prefer-precision-warning@+2 {{implicit conversion loses floating-point precision: 'double' to 'float'}}
// precision-only-warning@+1 {{implicit conversion loses floating-point precision: 'double' to 'float'}}
float d = 2.1; // size-only-warning {{implicit conversion between floating point types of different sizes}}
});
});
return 0;
}
8 changes: 4 additions & 4 deletions sycl/test/basic_tests/stdcpp_compat.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
// RUN: %clangxx -std=c++14 -fsycl -Wall -pedantic -Wno-c99-extensions -Wno-deprecated -fsyntax-only -Xclang -verify=expected,cxx14 %s -c -o %t.out
// RUN: %clangxx -std=c++14 -fsycl -Wall -pedantic -Wno-c99-extensions -Wno-deprecated -fsyntax-only -Xclang -verify %s -c -o %t.out -DSYCL_DISABLE_CPP_VERSION_CHECK_WARNING=1
// RUN: %clangxx -std=c++14 -fsycl --no-system-header-prefix=CL/sycl --no-system-header-prefix=sycl -Wall -pedantic -Wno-c99-extensions -Wno-deprecated -fsyntax-only -Xclang -verify=cxx14,warning_extension,expected %s -c -o %t.out
// RUN: %clangxx -std=c++17 -fsycl --no-system-header-prefix=CL/sycl --no-system-header-prefix=sycl -Wall -pedantic -Wno-c99-extensions -Wno-deprecated -fsyntax-only -Xclang -verify %s -c -o %t.out
// RUN: %clangxx -std=c++20 -fsycl --no-system-header-prefix=CL/sycl --no-system-header-prefix=sycl -Wall -pedantic -Wno-c99-extensions -Wno-deprecated -fsyntax-only -Xclang -verify %s -c -o %t.out
// RUN: %clangxx -fsycl --no-system-header-prefix=CL/sycl --no-system-header-prefix=sycl -Wall -pedantic -Wno-c99-extensions -Wno-deprecated -fsyntax-only -Xclang -verify %s -c -o %t.out
// RUN: %clangxx -std=c++14 -fsycl -Wall -pedantic -Wno-c99-extensions -Wno-deprecated -fsyntax-only -Xclang -verify=cxx14,warning_extension,expected %s -c -o %t.out
// RUN: %clangxx -std=c++17 -fsycl -Wall -pedantic -Wno-c99-extensions -Wno-deprecated -fsyntax-only -Xclang -verify %s -c -o %t.out
// RUN: %clangxx -std=c++20 -fsycl -Wall -pedantic -Wno-c99-extensions -Wno-deprecated -fsyntax-only -Xclang -verify %s -c -o %t.out
// RUN: %clangxx -fsycl -Wall -pedantic -Wno-c99-extensions -Wno-deprecated -fsyntax-only -Xclang -verify %s -c -o %t.out

// The test checks SYCL headers C++ compiance and that a warning is emitted
// when compiling in < C++17 mode.
Expand Down