[Driver][SYCL] For SYCL compilations, force C++ source even with .c #2491

mdtoguchi · 2020-09-17T20:51:49Z

When compiling for SYCL, we want to enforce C++ compilations. For all C
source file types, enforce C++.

Also move the -x c check to be in line with the other -x processing

When compiling for SYCL, we want to enforce C++ compilations. For all C source file types, enforce C++. Also move the -x c check to be in line with the other -x processing

keryell · 2020-09-17T21:13:01Z

What is the motivation for this or the use-case?
If you have real C code it will break the mangling, you cannot use SYCL in a TU except if it is actually C++.

mdtoguchi · 2020-09-17T21:25:10Z

@keryell, @Fznamznon is better suited to answer here.

mdtoguchi · 2020-09-21T15:16:18Z

@keryell, @Fznamznon is better suited to answer here.

@premanandrao, @erichkeane, I didn't realize @Fznamznon was out - can you respond? There is bit of urgency for this one for MKL.

erichkeane

2 small things. I'm not the maintainer here, but otherwise this LGTM.

erichkeane · 2020-09-21T15:21:51Z

clang/include/clang/Basic/DiagnosticDriverKinds.td

@@ -273,7 +273,7 @@ def err_drv_expecting_fopenmp_with_fopenmp_targets : Error<
 def err_drv_expecting_fsycl_with_sycl_opt : Error<
  "The option %0 must be used in conjunction with -fsycl to enable offloading.">;
 def err_drv_fsycl_with_c_type : Error<
-  "The option %0%1 must not be used in conjunction with -fsycl which expects C++ source.">;
+  "The option '%0' must not be used in conjunction with '-fsycl' which expects C++ source.">;


Nit: My reading would also add a comma after '-fsycl'

erichkeane · 2020-09-21T15:22:56Z

clang/lib/Driver/Driver.cpp

@@ -2443,9 +2441,21 @@ void Driver::BuildInputs(const ToolChain &TC, DerivedArgList &Args,
          if (const char *Ext = strrchr(Value, '.'))
            Ty = TC.LookupTypeForExtension(Ext + 1);

+          // For SYCL, convert C-type sources to C++-type sources.
+          if (IsSYCL) {


Should this be a switch instead? I would fear this getting unwieldy otherwise in pulldown/etc or if this got any more complicated.

- add comma to diagnostic - use switch statement instead of ifs

elizabethandrews

The changes LGTM but I do not know the answer to @keryell's concerns i.e. the motivation for this and incompatibilities between C and C++. I guess it will crash/error on real C code, which is fine? @premanandrao could you please take a look?

premanandrao · 2020-09-21T18:55:04Z

What is the motivation for this or the use-case?
If you have real C code it will break the mangling, you cannot use SYCL in a TU except if it is actually C++.

The main motivation appears to be the inline functions in C. These get emitted on device even when not used because of C rules for inline.

Example:

inline int foo() { return 5; }
int foo();

Previously we would treat this as a C file, if its file-name were .c with -fsycl. Now we would treat this as a C++ file with -fsycl, unless explicitly overridden with -x c.

AGindinson · 2020-09-21T19:09:17Z

Previously we would treat this as a C file, if its file-name were .c with -fsycl. Now we would treat this as a C++ file with -fsycl, unless explicitly overridden with -x c.

As far as I can judge from the new code (and from the earlier patches), -x c would not be permitted in SYCL mode.

premanandrao · 2020-09-21T19:12:23Z

Previously we would treat this as a C file, if its file-name were .c with -fsycl. Now we would treat this as a C++ file with -fsycl, unless explicitly overridden with -x c.

As far as I can judge from the new code (and from the earlier patches), -x c would not be permitted in SYCL mode.

You are right about the driver; I was testing with the cc1 compiler.

AGindinson · 2020-09-21T20:05:30Z

What is the motivation for this or the use-case?
If you have real C code it will break the mangling, you cannot use SYCL in a TU except if it is actually C++.

@keryell, AFAIU, the following background/justification come into play here:

C function emitting rules being different from C++, there have been cases where device code became invalidated by stuff like FE-generated variadic functions
The most common use-case for .c files is the client code performing a C -> C++ -> SYCL/DPC++ switch without any extension renaming taking place. In this scenario, the "legacy" file extension doesn't play any role, as it's essentially treated as a C++ input that had been striped of all "real C" code.

AGindinson · 2020-09-21T20:55:08Z

clang/lib/Driver/Driver.cpp

+            switch (Ty) {
+            case types::TY_C:
+              Ty = types::TY_CXX;
+              break;
+            case types::TY_CHeader:
+              Ty = types::TY_CXXHeader;
+              break;
+            case types::TY_PP_C:
+              Ty = types::TY_PP_CXX;
+              break;
+            case types::TY_PP_CHeader:
+              Ty = types::TY_PP_CXXHeader;
+              break;
+            default:
+              break;
+            }


Never thought I'd say this as a Python hater, but it's a shame we don't have something really analogous to a Python dict :D Take any alternative like a std::map - it's too verbose and therefore meaningless for a 4-case switch...

In C++23 I hope the pattern matching proposal https://wg21.link/p1371 will be voted in!
Python hater? Do not forget that C++23 is just Python, but running at 100x the speed. :-)

keryell · 2020-09-22T05:25:41Z

2. The most common use-case for `.c` files is the client code performing a C -> C++ -> SYCL/DPC++ switch without any extension renaming taking place. In this scenario, the "legacy" file extension doesn't play any role, as it's essentially treated as a C++ input that had been striped of all "real C" code.

I guess that if a user is owning an HPC machine, this is not to compile with -O0 to skip any deadcode elimination... :-)

AGindinson · 2020-09-22T08:38:09Z

2. The most common use-case for `.c` files is the client code performing a C -> C++ -> SYCL/DPC++ switch without any extension renaming taking place. In this scenario, the "legacy" file extension doesn't play any role, as it's essentially treated as a C++ input that had been striped of all "real C" code.

I guess that if a user is owning an HPC machine, this is not to compile with -O0 to skip any deadcode elimination... :-)

That's generally accurate, however there's a counter-example: we have early optimizations force-disabled for FPGA HW targets. I don't know if DCE gets run at the BE stage, but the risk remains.

When @Fznamznon's back online, she should be able to provide more examples of "true C" behavior being problematic.

This is a follow-up to 08884df Original commit: KhronosGroup/SPIRV-LLVM-Translator@89b19c46b5aaa2c

[Driver][SYCL] For SYCL compilations, force C++ source even with .c

e6cdfdf

When compiling for SYCL, we want to enforce C++ compilations. For all C source file types, enforce C++. Also move the -x c check to be in line with the other -x processing

mdtoguchi requested review from AGindinson, elizabethandrews, Fznamznon and premanandrao as code owners September 17, 2020 20:51

[Driver][SYCL] Add diagnostic for -std='C' with -fsycl

701e111

erichkeane reviewed Sep 21, 2020

View reviewed changes

Address review comments

d9cb5f4

- add comma to diagnostic - use switch statement instead of ifs

erichkeane approved these changes Sep 21, 2020

View reviewed changes

elizabethandrews reviewed Sep 21, 2020

View reviewed changes

AGindinson approved these changes Sep 21, 2020

View reviewed changes

elizabethandrews approved these changes Sep 21, 2020

View reviewed changes

romanovvlad merged commit adc2ac7 into intel:sycl Sep 22, 2020

jsji pushed a commit that referenced this pull request Apr 18, 2024

Deprecate OpLessOrGreater in favor of OpFOrdNotEqual (#2491)

01ceb0c

This is a follow-up to 08884df Original commit: KhronosGroup/SPIRV-LLVM-Translator@89b19c46b5aaa2c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Driver][SYCL] For SYCL compilations, force C++ source even with .c #2491

[Driver][SYCL] For SYCL compilations, force C++ source even with .c #2491

Uh oh!

mdtoguchi commented Sep 17, 2020

Uh oh!

keryell commented Sep 17, 2020

Uh oh!

mdtoguchi commented Sep 17, 2020

Uh oh!

mdtoguchi commented Sep 21, 2020

Uh oh!

erichkeane left a comment

Uh oh!

erichkeane Sep 21, 2020

Uh oh!

erichkeane Sep 21, 2020

Uh oh!

elizabethandrews left a comment

Uh oh!

premanandrao commented Sep 21, 2020

Uh oh!

AGindinson commented Sep 21, 2020

Uh oh!

premanandrao commented Sep 21, 2020

Uh oh!

AGindinson commented Sep 21, 2020

Uh oh!

AGindinson Sep 21, 2020 •

edited

Loading

Uh oh!

keryell Sep 22, 2020 •

edited

Loading

Uh oh!

keryell commented Sep 22, 2020

Uh oh!

AGindinson commented Sep 22, 2020

Uh oh!

Uh oh!

[Driver][SYCL] For SYCL compilations, force C++ source even with .c #2491

[Driver][SYCL] For SYCL compilations, force C++ source even with .c #2491

Uh oh!

Conversation

mdtoguchi commented Sep 17, 2020

Uh oh!

keryell commented Sep 17, 2020

Uh oh!

mdtoguchi commented Sep 17, 2020

Uh oh!

mdtoguchi commented Sep 21, 2020

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

erichkeane Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

erichkeane Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

premanandrao commented Sep 21, 2020

Uh oh!

AGindinson commented Sep 21, 2020

Uh oh!

premanandrao commented Sep 21, 2020

Uh oh!

AGindinson commented Sep 21, 2020

Uh oh!

AGindinson Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keryell Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keryell commented Sep 22, 2020

Uh oh!

AGindinson commented Sep 22, 2020

Uh oh!

Uh oh!

AGindinson Sep 21, 2020 •

edited

Loading

keryell Sep 22, 2020 •

edited

Loading