This is an archive of the discontinued LLVM Phabricator instance.

OpenCL: Improve vector printf warnings
ClosedPublic

Authored by arsenm on Nov 28 2018, 1:27 PM.

Details

Reviewers
Anastasia
Summary

The vector modifier is considered separate, so
don't treat it as a conversion specifier.

This is still not warning on some cases, like
using a type that isn't a valid vector element.

Diff Detail

Event Timeline

arsenm created this revision.Nov 28 2018, 1:27 PM
Anastasia added inline comments.Nov 29 2018, 3:22 AM
test/SemaOpenCL/format-strings-fixit.cl
13

Does this not work yet?

test/SemaOpenCL/printf-format-strings.cl
65

So there is no way to print vector of float? What will happen on architectures that don't support doubles?

I guess it's the same for printf in general with the float type?

arsenm marked 2 inline comments as done.Nov 29 2018, 8:12 AM
arsenm added inline comments.
test/SemaOpenCL/format-strings-fixit.cl
13

It does. This was a leftover reference I was using I can remove

test/SemaOpenCL/printf-format-strings.cl
65

There is, it's converted to a vector of doubles. This case warns because the element count mismatches

arsenm marked an inline comment as done and an inline comment as not done.Nov 29 2018, 8:14 AM
arsenm added inline comments.
test/SemaOpenCL/printf-format-strings.cl
65

The conversion is to float if doubles aren't supported

arsenm marked an inline comment as done.Nov 29 2018, 8:19 AM
arsenm added inline comments.
test/SemaOpenCL/printf-format-strings.cl
65

The warning message is wrong though without doubles, and still says double.

Anastasia added inline comments.Nov 29 2018, 8:52 AM
test/SemaOpenCL/printf-format-strings.cl
65

Can we add this explicitly to the test just for a record? I guess the warning message can't be changed to print the type correctly?

Also would it make sense to test other builtin types: double, char...?

arsenm marked an inline comment as done.Nov 29 2018, 9:56 AM
arsenm added inline comments.
test/SemaOpenCL/printf-format-strings.cl
65

This should be easy to fix, but the way to test for doubles is enabled is making this unnecessarily difficult. It requires threading OpenCLOptions through all of the relevant functions to check for cl_khr_fp64. Why is this separate from LangOptions?

arsenm updated this revision to Diff 175920.Nov 29 2018, 11:55 AM

Add tests without fp64

Anastasia accepted this revision.Nov 30 2018, 4:17 AM

LGTM! Thanks!

test/SemaOpenCL/printf-format-strings.cl
65

I see. I think the problem is that it is set conditionally based on the targets in earlier CL versions. So it had to be a separated into target specific option. :(

This revision is now accepted and ready to land.Nov 30 2018, 4:17 AM
arsenm marked an inline comment as done.Nov 30 2018, 10:49 AM
arsenm added inline comments.
test/SemaOpenCL/printf-format-strings.cl
65

Would it be reasonable to add a new HasNoDouble field or something in LangOptions?

arsenm closed this revision.Dec 1 2018, 2:19 PM

r348084