This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Allow -std={cl|CL}{|1.1|1.2|2.0} in driver
ClosedPublic

Authored by yaxunl on May 25 2016, 9:13 AM.

Details

Summary

Fix a regression which forbids using -std=cl|CL1.1|CL1.2|CL2.0 in driver.

Allow -std and -cl-std={cl|CL}{|1.1|1.2|2.0}.

Diff Detail

Event Timeline

yaxunl updated this revision to Diff 58436.May 25 2016, 9:13 AM
yaxunl retitled this revision from to [OpenCL] Allow -std=CL|CL1.1|CL1.2|CL2.0 in driver.
yaxunl updated this object.
yaxunl added a reviewer: Anastasia.
yaxunl added a subscriber: yaxunl.May 25 2016, 12:08 PM

I will fix that. Thanks.

Sam

From: metafoo@gmail.com [mailto:metafoo@gmail.com] On Behalf Of Richard Smith
Sent: Wednesday, May 25, 2016 2:54 PM
To: Liu, Yaxun (Sam) <Yaxun.Liu@amd.com>; reviews+D20630+public+1c58d99d1f368a1e@reviews.llvm.org
Cc: alexey.bader@intel.com; Anastasia Stulova <anastasia.stulova@arm.com>; cfe-commits <cfe-commits@lists.llvm.org>
Subject: Re: [PATCH] D20630: [OpenCL] Allow -std=CL|CL1.1|CL1.2|CL2.0 in driver

yaxunl updated this revision to Diff 58471.May 25 2016, 12:33 PM
yaxunl retitled this revision from [OpenCL] Allow -std=CL|CL1.1|CL1.2|CL2.0 in driver to [OpenCL] Allow -std=cl|CL1.1|CL1.2|CL2.0 in driver.
yaxunl updated this object.

Revised as Richard suggested.

rsmith added inline comments.May 25 2016, 1:49 PM
lib/Frontend/CompilerInvocation.cpp
1587–1589

How about changing these to the lowercase form too, and treating the uppercase versions as (deprecated) synonyms? (And likewise changing LangStandards.def to list the lowercase versions, perhaps with the uppercase versions as aliases.)

yaxunl added inline comments.May 25 2016, 2:43 PM
lib/Frontend/CompilerInvocation.cpp
1587–1589

-cl-std=CL1.1|CL1.2|CL2.0 is defined by OpenCL spec. Allowing lower case may cause some confusion.

-cl-std=cl is not part of OpenCL spec.

How about keeping all -cl-std= options big letters and all -std= options small letters?

rsmith added inline comments.May 25 2016, 3:00 PM
lib/Frontend/CompilerInvocation.cpp
1587–1589

What? The OpenCL spec does not get to dictate our command-line argument syntax. If they think they do, they're just mistaken.

Anastasia added inline comments.May 26 2016, 7:00 AM
lib/Frontend/CompilerInvocation.cpp
1587–1589

OpenCL spec s5.8.4.5 standardizes the compiler option controlling the OpenCL C version to be as Sam mentioned above:

-cl-std=CL1.1|CL1.2|CL2.0

But this is a part of driver compilation API, and surely doesn't have anything to do with standalone Clang itself.

However, I must say using uppercase letters for CL version is quite common in OpenCL community while using lowercase format is more common for C community.

I don't have much preference here to be honest, as soon as we are consistent in one way or another.

yaxunl updated this revision to Diff 58978.May 30 2016, 10:26 AM
yaxunl retitled this revision from [OpenCL] Allow -std=cl|CL1.1|CL1.2|CL2.0 in driver to [OpenCL] Allow -std={cl|CL}{|1.1|1.2|2.0} in driver.
yaxunl updated this object.

Revised as Richard suggested.

Anastasia accepted this revision.Jun 1 2016, 8:49 AM
Anastasia edited edge metadata.

LGTM! Thanks!

This revision is now accepted and ready to land.Jun 1 2016, 8:49 AM
yaxunl marked 4 inline comments as done.Jun 13 2016, 7:37 AM

Ping!

Richard, are you OK with this? Thanks.

Ping! It would be nice to have this fix committed soon!

Thanks!

This revision was automatically updated to reflect the committed changes.