[SYCL] Add option -fsycl-use-integration-headers #14759

premanandrao · 2024-07-25T00:14:07Z

Add option -fsycl-use-integration-headers to enable integration between device and host compilation phases using integration headers and footers. This is currently the default behavior.

The option is passed through to the cc1 phase along with a macro __INTEL_SYCL_USE_INTEGRATION_HEADERS.

clang/include/clang/Driver/Options.td

tahonermann

See comments; I don't understand why the option is considered related to code gen.

I still don't love the option name. The option controls two major behaviors:

Disabling the generation of integration headers and associated options used in generated compiler actions.
Setting the macro the SYCL run-time library will check to determine how to access kernel information.

Perhaps we shouldn't use a single option to influence both of these behaviors. How about this approach?

Rename the option to -f[no-]sycl-integration-headers to enable/disable generation and use of SYCL integration headers in the generated compiler actions. This option won't be upstreamed.
Have the SYCL run-time library check to see if the builtin functions are available (via has_builtin or has_feature or similar) and, if so, default to using the builtins unless the __INTEL_SYCL_USE_INTEGRATION_HEADERS macro is defined.
Modify the dpcpp/icx drivers to predefine __INTEL_SYCL_USE_INTEGRATION_HEADERS until we are ready to change the default behavior for SYCL aware host compilers. Continue to predefine the macro for SYCL unaware third party host compilers indefinitely. For testing, we can undefine the macro on the command line.
Allow the user to manually define the macro on the command line as a workaround for any backward compatibility issues that arise.

clang/include/clang/Basic/DiagnosticDriverKinds.td

clang/include/clang/Driver/Options.td

clang/include/clang/Basic/CodeGenOptions.def

clang/lib/Driver/Driver.cpp

clang/test/Driver/sycl-use-int-headers.cpp

clang/lib/Driver/Driver.cpp

clang/include/clang/Driver/Options.td

clang/test/Driver/sycl-use-int-headers.cpp

clang/lib/Driver/Driver.cpp

clang/include/clang/Driver/Options.td

tahonermann

I added some minor comments and suggestions. The new updates look good to me.

I might have more comments to add depending on whether we can figure out how to get the BoolFOption approach to work with the new option definition.

tahonermann · 2024-08-16T01:46:54Z

clang/include/clang/Basic/LangOptions.def

+LANGOPT(SYCLUseIntegrationHeaders, 1, 1, "Use integration headers for "
+        "communication between device and host compilation phases")


I think this is fine. Here is another suggestion; use which ever one you prefer.

Suggested change

LANGOPT(SYCLUseIntegrationHeaders, 1, 1, "Use integration headers for "

"communication between device and host compilation phases")

LANGOPT(SYCLUseIntegrationHeaders, 1, 1, "Use integration headers for "

"coordination of device and host compilation phases")

I decided to leave the old wording in place.

clang/lib/Driver/Driver.cpp

clang/test/Driver/sycl-use-int-headers.cpp

clang/lib/Driver/Driver.cpp

tahonermann

The changes to use BoolFOption for the option definition looks great. Requesting changes to deal with the default use-integration-header behavior being specified in multiple locations. I added a couple of other minor comments as well.

tahonermann · 2024-08-26T15:32:36Z

clang/include/clang/Driver/Options.td

+      " that integration headers be used for "
+      "communication between the host and the device compilations. SYCL Only">>;


For pedantic consistency, I suggest:

Suggested change

" that integration headers be used for "

"communication between the host and the device compilations. SYCL Only">>;

" that integration headers be used for"

" communication between the host and the device compilations. SYCL Only">>;

tahonermann · 2024-08-26T16:16:11Z

clang/lib/Driver/Driver.cpp

+    // -fno-sycl-use-integration-headers cannot be used with
+    // -fsycl-host-compiler.
+    if (SYCLHostCompiler && SYCLUIHArg->getOption().matches(
+                                options::OPT_fno_sycl_use_integration_headers))
+      Diag(clang::diag::err_drv_option_conflict)
+          << SYCLHostCompiler->getSpelling().split('=').first
+          << "-fno-sycl-use-integration-headers";
+  }


The use of getLastArg() above is good since it ensures that the err_drv_sycl_opt_requires_opt diagnostic is only issued when one of the options is explicitly used. I think we can simplify the check for conflict with the host compiler option a bit though and, while we're add it, perhaps future proof the code a bit against a change of default behavior. I suggest colocating the following with the declarations of HasSYCL and HasValidSYCLRuntime:

bool UseSYCLIntegrationHeaders = Args.hasFlag(options::OPT_fsycl_use_integration_headers, options::OPT_fno_sycl_use_integration_headers, true);

Then, break this diagnostic out of the enclosing if statement:

Suggested change

// -fno-sycl-use-integration-headers cannot be used with

// -fsycl-host-compiler.

if (SYCLHostCompiler && SYCLUIHArg->getOption().matches(

options::OPT_fno_sycl_use_integration_headers))

Diag(clang::diag::err_drv_option_conflict)

<< SYCLHostCompiler->getSpelling().split('=').first

<< "-fno-sycl-use-integration-headers";

}

}

// -fno-sycl-use-integration-headers cannot be used with

// -fsycl-host-compiler.

if (SYCLHostCompiler && !UseSYCLIntegrationHeaders) {

Diag(clang::diag::err_drv_option_conflict)

<< SYCLHostCompiler->getSpelling().split('=').first

<< "-fno-sycl-use-integration-headers";

}

tahonermann · 2024-08-26T16:19:23Z

clang/lib/Driver/Driver.cpp

+            if (!Args.hasFlag(options::OPT_fsycl_use_integration_headers,
+                              options::OPT_fno_sycl_use_integration_headers,
+                              true))


The default value of true is passed in multiple places (here, in Driver::BuildActions(), in Clang::ConstructHostCompilerJob(), in Clang::ConstructJob(), and in the suggested change I offered in a previous comment). Perhaps we should define the default in one place somewhere. There are a few different approaches. I don't have a strong opinion on any of these, but I think we should do something.

We could introduce a constant that is passed in each of the calls to hasFlag().

We could introduce a function to be called at each of these locations to ascertain whether integration headers are being used; that function would query the options and supply the default.

We could check the options in one place and store that result.

I went with option 2. Thanks @tahonermann

Great! It looks like this particular location didn't get updated to use it though.

Add option -fsycl-use-builtins-for-integration to enable integration between device and host compilation phases without using integration headers and footers. The option is passed through to the cc1 phase along with a macro __INTEL_SYCL_USE_BUILTINS_FOR_INTEGRATION.

…with option

premanandrao · 2024-09-11T19:44:32Z

And apologies; there were merge conflicts and I had to do a force push after resolving them.

tahonermann

This is looking good. I think I spotted a couple of minor issues.

tahonermann · 2024-09-19T19:23:44Z

clang/include/clang/Driver/Options.td

+defm sycl_use_integration_headers : BoolFOption<"sycl-use-integration-headers",
+      LangOpts<"SYCLUseIntegrationHeaders">, DefaultTrue,
+      PosFlag<SetTrue, [], [ClangOption, CLOption], "Specify">,
+      NegFlag<SetFalse, [], [ClangOption, CLOption], "Do not specify">,
+      BothFlags<[HelpHidden], [ClangOption, CLOption, CC1Option],
+      " that integration headers be used for communication"
+      " between the host and the device compilations. SYCL Only">>;


Inconsistent indentation with respect to surrounding code.

Suggested change

defm sycl_use_integration_headers : BoolFOption<"sycl-use-integration-headers",

LangOpts<"SYCLUseIntegrationHeaders">, DefaultTrue,

PosFlag<SetTrue, [], [ClangOption, CLOption], "Specify">,

NegFlag<SetFalse, [], [ClangOption, CLOption], "Do not specify">,

BothFlags<[HelpHidden], [ClangOption, CLOption, CC1Option],

" that integration headers be used for communication"

" between the host and the device compilations. SYCL Only">>;

defm sycl_use_integration_headers : BoolFOption<"sycl-use-integration-headers",

LangOpts<"SYCLUseIntegrationHeaders">, DefaultTrue,

PosFlag<SetTrue, [], [ClangOption, CLOption], "Specify">,

NegFlag<SetFalse, [], [ClangOption, CLOption], "Do not specify">,

BothFlags<[HelpHidden], [ClangOption, CLOption, CC1Option],

" that integration headers be used for communication"

" between the host and the device compilations. SYCL Only">>;

tahonermann · 2024-09-19T19:37:53Z

clang/lib/Driver/Driver.cpp

+  if (SYCLUIHArg) {
+    // -f[no-]sycl-use-integration-headers cannot be used without
+    // -fsycl.  (-fintelfpga implies -fsycl).
+    if (!HasSYCL && !SYCLfpga)


Is there any reason not to include the test of SYCLfpga in the initialization of HasSYCL above? As is, it looks like HasValidSYCLRuntime will be initialized to false if -fintelfpga is passed (but -fsycl is not).

tahonermann · 2024-09-19T19:39:49Z

clang/lib/Driver/Driver.cpp

+            if (!Args.hasFlag(options::OPT_fsycl_use_integration_headers,
+                              options::OPT_fno_sycl_use_integration_headers,
+                              true))


Great! It looks like this particular location didn't get updated to use it though.

tahonermann · 2024-09-19T19:41:27Z

clang/lib/Driver/Driver.cpp

+      // If -fno-sycl-use-integration-headers is speficified, don't generate
+      // integration headers and footers.
+      if (!Args.hasFlag(options::OPT_fsycl_use_integration_headers,
+                        options::OPT_fno_sycl_use_integration_headers, true))
+        continue;


Suggested change

// If -fno-sycl-use-integration-headers is speficified, don't generate

// integration headers and footers.

if (!Args.hasFlag(options::OPT_fsycl_use_integration_headers,

options::OPT_fno_sycl_use_integration_headers, true))

continue;

// If -fno-sycl-use-integration-headers is speficified, don't generate

// integration headers and footers.

if (!getSYCLUseIntegrationHeaders(Args))

continue;

premanandrao · 2024-09-23T16:53:48Z

@tahonermann, when you have time, could you please review this again?

tahonermann · 2024-09-24T18:52:16Z

@tahonermann, when you have time, could you please review this again?

@premanandrao, it looks like the latest changes are still what I reviewed 5 days ago :)

premanandrao requested review from a team as code owners July 25, 2024 00:14

premanandrao temporarily deployed to WindowsCILock July 25, 2024 00:14 — with GitHub Actions Inactive

mdtoguchi reviewed Jul 25, 2024

View reviewed changes

clang/include/clang/Driver/Options.td Outdated Show resolved Hide resolved

premanandrao temporarily deployed to WindowsCILock July 25, 2024 01:04 — with GitHub Actions Inactive

tahonermann reviewed Jul 25, 2024

View reviewed changes

premanandrao changed the title ~~[SYCL] Add option -fsycl-use-builtins-for-integration~~ [SYCL] Add option -fsycl-use-integration-headers Jul 31, 2024

premanandrao temporarily deployed to WindowsCILock July 31, 2024 22:14 — with GitHub Actions Inactive

premanandrao temporarily deployed to WindowsCILock August 1, 2024 00:38 — with GitHub Actions Inactive

tahonermann reviewed Aug 7, 2024

View reviewed changes

clang/test/Driver/sycl-use-int-headers.cpp Outdated Show resolved Hide resolved

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved

clang/include/clang/Driver/Options.td Outdated Show resolved Hide resolved

clang/test/Driver/sycl-use-int-headers.cpp Show resolved Hide resolved

mdtoguchi reviewed Aug 15, 2024

View reviewed changes

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved

clang/include/clang/Driver/Options.td Outdated Show resolved Hide resolved

tahonermann reviewed Aug 16, 2024

View reviewed changes

premanandrao temporarily deployed to WindowsCILock August 16, 2024 19:55 — with GitHub Actions Inactive

premanandrao temporarily deployed to WindowsCILock August 16, 2024 21:33 — with GitHub Actions Inactive

premanandrao had a problem deploying to WindowsCILock August 21, 2024 18:35 — with GitHub Actions Error

premanandrao temporarily deployed to WindowsCILock August 21, 2024 18:49 — with GitHub Actions Inactive

premanandrao temporarily deployed to WindowsCILock August 21, 2024 19:31 — with GitHub Actions Inactive

mdtoguchi reviewed Aug 21, 2024

View reviewed changes

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved

premanandrao temporarily deployed to WindowsCILock August 22, 2024 13:59 — with GitHub Actions Inactive

premanandrao temporarily deployed to WindowsCILock August 22, 2024 15:07 — with GitHub Actions Inactive

premanandrao temporarily deployed to WindowsCILock August 22, 2024 21:16 — with GitHub Actions Inactive

premanandrao temporarily deployed to WindowsCILock August 22, 2024 22:21 — with GitHub Actions Inactive

premanandrao had a problem deploying to WindowsCILock August 26, 2024 02:10 — with GitHub Actions Error

premanandrao temporarily deployed to WindowsCILock August 26, 2024 02:16 — with GitHub Actions Inactive

premanandrao temporarily deployed to WindowsCILock August 26, 2024 02:52 — with GitHub Actions Inactive

tahonermann suggested changes Aug 26, 2024

View reviewed changes

premanandrao added 4 commits September 9, 2024 10:25

Address code review comments

3979ced

Address code review comments

c12704d

address more code review comments

b63aacc

premanandrao added 12 commits September 9, 2024 10:25

Missed one CR comment

a925c95

Make it a BoolFOption

eb79125

Resolve merge conflicts

6b23ea1

Address CR comments; particularly disabling header/footer generation …

10e7844

…with option

Make formatter happy

64a0d5a

Remove trailing whitespaces

bd2b13d

Use hasFlag instead; multiple places

ed894d0

Fix clang-format issues

5491644

Fix clang-format issues

838bb64

Resolve conflicts after rebase

884a5e4

Fix conflict - one more change

ed7b026

Merge conflicts; address CR comments

fa6ae5d

premanandrao force-pushed the remote_use_builtins branch from 5e652c4 to fa6ae5d Compare September 11, 2024 18:38

premanandrao had a problem deploying to WindowsCILock September 11, 2024 18:39 — with GitHub Actions Error

clang-format issues

8fbdad8

premanandrao had a problem deploying to WindowsCILock September 11, 2024 19:00 — with GitHub Actions Error

Move the options logic to Driver.h

1a83113

premanandrao temporarily deployed to WindowsCILock September 11, 2024 19:39 — with GitHub Actions Inactive

premanandrao temporarily deployed to WindowsCILock September 11, 2024 21:57 — with GitHub Actions Inactive

tahonermann suggested changes Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Add option -fsycl-use-integration-headers #14759

[SYCL] Add option -fsycl-use-integration-headers #14759

premanandrao commented Jul 25, 2024 •

edited

Loading

tahonermann left a comment

tahonermann left a comment

tahonermann Aug 16, 2024

premanandrao Aug 21, 2024

tahonermann left a comment

tahonermann Aug 26, 2024

premanandrao Sep 11, 2024

tahonermann Aug 26, 2024

premanandrao Sep 11, 2024

tahonermann Aug 26, 2024

premanandrao Sep 11, 2024

tahonermann Sep 19, 2024

premanandrao commented Sep 11, 2024

tahonermann left a comment

tahonermann Sep 19, 2024

tahonermann Sep 19, 2024

tahonermann Sep 19, 2024

tahonermann Sep 19, 2024

premanandrao commented Sep 23, 2024

tahonermann commented Sep 24, 2024

		LANGOPT(SYCLUseIntegrationHeaders, 1, 1, "Use integration headers for "
		"communication between device and host compilation phases")

		" that integration headers be used for "
		"communication between the host and the device compilations. SYCL Only">>;

[SYCL] Add option -fsycl-use-integration-headers #14759

Are you sure you want to change the base?

[SYCL] Add option -fsycl-use-integration-headers #14759

Conversation

premanandrao commented Jul 25, 2024 • edited Loading

tahonermann left a comment

Choose a reason for hiding this comment

tahonermann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tahonermann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

premanandrao commented Sep 11, 2024

tahonermann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

premanandrao commented Sep 23, 2024

tahonermann commented Sep 24, 2024

premanandrao commented Jul 25, 2024 •

edited

Loading