This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Improve floating point literal handling
ClosedPublic

Authored by neil.hickey on Sep 5 2016, 7:53 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

neil.hickey updated this revision to Diff 70332.Sep 5 2016, 7:53 AM
neil.hickey retitled this revision from to [OpenCL] Improve double literal handling.
neil.hickey updated this object.
neil.hickey added a reviewer: Anastasia.
neil.hickey added a subscriber: cfe-commits.
Anastasia edited edge metadata.Sep 5 2016, 9:02 AM

Could we add a CodeGen test as well to check that the constants generated are in the right precision format?

neil.hickey updated this revision to Diff 70527.Sep 7 2016, 6:50 AM
neil.hickey edited edge metadata.

Added a CodeGen test as well as fixed an issue with vararg type promotion

Anastasia added inline comments.Sep 7 2016, 10:25 AM
lib/Sema/SemaExpr.cpp
832 ↗(On Diff #70527)

This should go on the previous line.

837 ↗(On Diff #70527)

Could we merge this and two lines above into one?

840 ↗(On Diff #70527)

I think the formatting is not right here.

Could you change to:

} else {
test/CodeGenOpenCL/fpmath.cl
28 ↗(On Diff #70527)

Could you please add a check that double is generated in either CL2.0 or fp64 mode.

neil.hickey updated this revision to Diff 70799.Sep 9 2016, 2:59 AM

Modifying formating to match LLVM style. Adding new test to confirm that the vararg for printf is cast up to a double is allowable.

neil.hickey added inline comments.Sep 9 2016, 3:00 AM
lib/Sema/SemaExpr.cpp
837 ↗(On Diff #70527)

This was the result when I ran this through clang-format so I'd rather keep it as it is.

Anastasia accepted this revision.Sep 9 2016, 11:55 AM
Anastasia edited edge metadata.

LGTM! Thanks!

This revision is now accepted and ready to land.Sep 9 2016, 11:55 AM
neil.hickey closed this revision.Sep 16 2016, 3:45 AM

Commit merged to trunk

committed @ 281714

neil.hickey added a reviewer: tstellarAMD.

There was a bug whereby an implicitcast was being applied from float to float, this caused issues later on in builin processing which caused an assertion. This changed patch removes the duplicate, superfluous, cast.

neil.hickey reopened this revision.Sep 19 2016, 9:05 AM
This revision is now accepted and ready to land.Sep 19 2016, 9:05 AM
yaxunl added inline comments.Sep 19 2016, 10:43 AM
lib/Sema/SemaExpr.cpp
3431 ↗(On Diff #71837)

This check

(getLangOpts().OpenCLVersion >= 120 &&
                  Context.getTargetInfo()
                      .getSupportedOpenCLOpts()
                      .cl_khr_fp64)

is redundant since for CL 1.2 and above getOpenCLOptions().cl_khr_fp64 is set to be true by default.

neil.hickey added inline comments.Sep 20 2016, 2:18 AM
lib/Sema/SemaExpr.cpp
3431 ↗(On Diff #71837)

This is getSupportedOpenCLOpts(). Some hardware may not support doubles

yaxunl added inline comments.Sep 20 2016, 7:48 AM
lib/Sema/SemaExpr.cpp
3431 ↗(On Diff #71837)

In Sema::Initialize(), there is code to initialize enabled extensions:

  // Initialize predefined OpenCL types and supported optional core features.
  if (getLangOpts().OpenCL) {
#define OPENCLEXT(Ext) \
     if (Context.getTargetInfo().getSupportedOpenCLOpts().is_##Ext##_supported_core( \
         getLangOpts().OpenCLVersion)) \
       getOpenCLOptions().Ext = 1;
#include "clang/Basic/OpenCLExtensions.def"

is_##Ext##_supported_core accounts for two factors: 1. whether the target supports the extension; 2. whether the extension has become OpenCL core featrue.

Since getOpenCLOptions().cl_khr_fp64 is initialized with the same value as in the mentioned check, the check is redundant.

neil.hickey updated this revision to Diff 76587.Nov 1 2016, 9:58 AM

Sorry for the delay. It looks like the code to handle extensions was changed since Neil Henning added the check against opencl 1.2. Perhaps the best approach is just to remove the check.

yaxunl added inline comments.Nov 7 2016, 8:55 AM
test/SemaOpenCL/extensions.cl
28 ↗(On Diff #76587)

expected-no-diagnostics applies to the whole file. better move to the beginning of the file.

otherwise LGTM. Thanks!

neil.hickey updated this revision to Diff 77321.Nov 9 2016, 2:03 AM

Moving location of no-diagnostics statement to beginning of file

svenvh added inline comments.Nov 10 2016, 3:11 AM
lib/Sema/SemaExpr.cpp
3431 ↗(On Diff #71837)

Your new patch no longer does version checks, so there is no need to mention it in the comments anymore I'd say.

Improving now confusing comment.

svenvh accepted this revision.Nov 11 2016, 1:49 AM
svenvh edited edge metadata.

LGTM!

This revision was automatically updated to reflect the committed changes.
neil.hickey edited edge metadata.
neil.hickey removed rL LLVM as the repository for this revision.

Fixes to tests and removal of incorrect check to stop float to float casts if types match. This was still needed as it was an lvalue to rvalue cast. Added extra code in SemaChecking to allow a float to float cast to appear and be handled.

neil.hickey reopened this revision.Nov 15 2016, 2:44 AM
This revision is now accepted and ready to land.Nov 15 2016, 2:44 AM
svenvh added inline comments.Nov 15 2016, 3:19 AM
lib/Sema/SemaChecking.cpp
3732 ↗(On Diff #77961)

The comment should mention float -> float now as well.

lib/Sema/SemaExpr.cpp
712 ↗(On Diff #77961)

You can change this back to what it was before your first commit (which also fixes the indentation).

neil.hickey retitled this revision from [OpenCL] Improve double literal handling to [OpenCL] Improve floating point literal handling.
neil.hickey updated this object.

Fixing indentation and improving comments.

Thanks, lgtm!

This revision was automatically updated to reflect the committed changes.