Page MenuHomePhabricator

[OpenCL] Fixes opencl.cl testcase issues and cl-strict-aliasing only allowed with cl-std=CL
ClosedPublic

Authored by ashi1 on Jul 8 2016, 2:05 PM.

Details

Summary

Fixes failures in test/Driver/opencl.cl.

Also fixes strict-aliasing option to only be allowed when OpenCL Version 1.0. Added testcase in test/Frontend/opencl-blocks.cl .

Diff Detail

Repository
rL LLVM

Event Timeline

ashi1 updated this revision to Diff 63314.Jul 8 2016, 2:05 PM
ashi1 retitled this revision from to [OpenCL] Fixes opencl.cl testcase issues and cl-strict-aliasing only allowed with cl-std=CL.
ashi1 updated this object.
ashi1 added reviewers: Anastasia, bkramer.
ashi1 set the repository for this revision to rL LLVM.
ashi1 added subscribers: nhaustov, rsmith, bader and 3 others.
yaxunl added inline comments.Jul 11 2016, 7:25 AM
test/Frontend/opencl-blocks.cl
9 ↗(On Diff #63314)

Better separate these tests to another file, e.g. cl-strict-aliasing.cl

Anastasia added inline comments.Jul 11 2016, 9:20 AM
lib/Frontend/CompilerInvocation.cpp
1681 ↗(On Diff #63314)

Was this code moved intensionally?

test/Frontend/opencl-blocks.cl
9 ↗(On Diff #63314)

I think it was right to have this in test/Driver/opencl.cl. Why was it moved in here?

yaxunl added inline comments.Jul 11 2016, 9:29 AM
test/Frontend/opencl-blocks.cl
9 ↗(On Diff #63314)

This is a frontend option and the warning is emitted by frontend, not the driver. The driver does not consume the option. It just passes it to the frontend.

The driver test is mostly testing whether the option is correctly passed to the compiler invocation by using -###.

ashi1 marked 4 inline comments as done.Jul 11 2016, 10:14 AM
ashi1 added inline comments.
lib/Frontend/CompilerInvocation.cpp
1681 ↗(On Diff #63314)

Yes this had to be moved intentionally, due to Opts.OpenCLVersion not being correctly set up until after the above function call to CompilerInvocation::setLangDefaults(

test/Frontend/opencl-blocks.cl
9 ↗(On Diff #63314)

should it be called opencl-strict-aliasing.cl or cl-strict-aliasing.cl

yaxunl added inline comments.Jul 11 2016, 10:48 AM
test/Frontend/opencl-blocks.cl
9 ↗(On Diff #63314)

either is OK.

ashi1 updated this revision to Diff 63539.Jul 11 2016, 10:57 AM
ashi1 marked 2 inline comments as done.

Moved cl-strict-aliasing testing to a new file cl-strict-aliasing.cl

Anastasia added inline comments.Jul 12 2016, 8:48 AM
test/Frontend/opencl-blocks.cl
9 ↗(On Diff #63314)

I would rather see fewer small test files populated and more compact/less fragmented testing. Is there any reason not put this with blocks and give it more generic name, let's say opencl.cl.

ashi1 updated this revision to Diff 63722.Jul 12 2016, 1:09 PM
ashi1 marked 4 inline comments as done.

Revised to Anastasia's comments. Removed opencl-blocks.cl, and created a generic opencl.cl inside test/Frontend folder.

This change passes clang-test.

Anastasia accepted this revision.Jul 13 2016, 8:34 AM
Anastasia edited edge metadata.

LGTM! Thanks!

This revision is now accepted and ready to land.Jul 13 2016, 8:34 AM
This revision was automatically updated to reflect the committed changes.

The test cfe/trunk/test/Frontend/opencl.cl that was added here appears to
fail.

Running "ninja check-clang" doesn't pick this up because
cfe/trunk/test/Frontend/lit.local.cfg doesn't contain '.cl' as a file
suffix:

config.suffixes = ['.c', '.cpp', '.m', '.mm', '.ll']

As soon as I add the suffix:

config.suffixes = ['.c', '.cpp', '.m', '.mm', '.ll', '.cl']

the test fails. The failure seems to happen because of the new
"CHECK_INVALID_OPENCL_VERSION" checks. For OpenCL versions 1.1 and 1.2,
compilation fails for these checks because of the expected error "blocks
support disabled".

A solution seems to be to add "-fblocks" to the command line for these two
checks.

I'll be sending a patch with this fix out for review shortly.

Fix submitted