[PATCH] D97717: [SYCL] Rework the SYCL driver options

Mon Mar 15 13:26:02 PDT 2021

aaron.ballman marked an inline comment as done.
aaron.ballman added inline comments.

================
Comment at: clang/unittests/Frontend/CompilerInvocationTest.cpp:547
+  // there is no syntax I could find that would allow it). However, the option
+  // is handled properly on a real invocation. See: Clang::ConstructJob().
+  ASSERT_THAT(GeneratedArgs, Contains(HasSubstr("-sycl-std=")));
----------------
jansvoboda11 wrote:
> The `generateCC1CommandLine` function is used when CC1 gets invoked with `-round-trip-args`. It is also going to be used in `clang-scan-deps`.
> 
> Instead of putting a fixme here, I think it would make more sense to decide how to handle the option properly. There are a couple of approaches:
> * Instead of removing `ShouldParseIf<fsycl.KeyPath>` from `sycl_std_EQ`, it could be replaced with `ShouldParseIf<!strconcat(fsycl_is_device.KeyPath, "||", fsycl_is_host.KeyPath)>`.
> * Remove `ShouldParseIf<fsycl.KeyPath>` from `sycl_std_EQ`, but issue warning/error in `CompilerInvocation::fixupInvocation` whenever `SYCLVersion != LangOptions::SYCL_None && !(SYCLIsDevice || SYCLIsHost)`.
> * Remove the marshalling annotation from `sycl_std_EQ` and handle its parsing/generation including the device/host checks manually in `CompilerInvocation`. (I'm personally not fan of moving simple logic like this from tablegen marshalling annotations to `CompilerInvocation` though.)
> 
> See https://clang.llvm.org/docs/InternalsManual.html#adding-new-command-line-option for more information.
Thank you for the suggestions! I took the first suggestion and applied it in this latest revision and I really like the results.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97717/new/

https://reviews.llvm.org/D97717