[HLSL] add -I option for dxc mode.

[python3kgae]

Summary

A new option -I is added for dxc mode.
It is just alias of existing cc1 -I option.

[pow2clk]

I'm confused by the ways this option is allowed to be specified

clang/include/clang/Driver/Options.td

6880

I'm not sure why this has the suffix "Conflict" In addition, you can't currently specify the "I" option with "--" in DXC.

clang/test/Driver/dxc_I.hlsl

2

test.h is kind of a funny name for a directory. 🤔 Maybe just call it "test"?

[beanz]

clang/include/clang/Driver/Options.td

6905

This option has the same behavior between DXC and clang, is there a reason we need to define a new option value instead of reusing the existing one?

[beanz]

clang/include/clang/Driver/Options.td

6905

As a note, the I_Group only has the -I flag, so we probably can just make I_Group valid for DXC.

[python3kgae]

Update test.

[python3kgae]

clang/include/clang/Driver/Options.td

6880

The conflict happened with clang-cl which use same prefix ("/", "-") as dxc.

def _SLASH_I : CLJoinedOrSeparate<"I">,

HelpText<"Add directory to include search path">, MetaVarName<"<dir>">,
Alias<I>;

6905

clang -I is CC1Option,CC1AsOption,FlangOption,FC1Option. We need one for dxc mode with prefix "/", "-". But it conflict with _SLASH_I for clang-cl.
So have to add dxc_I.

[beanz]

clang/include/clang/Driver/Options.td

6880

Our option’s behavior is the same as _SLASH_I, so why can’t we reuse it instead of re-implementing it.

[python3kgae]

clang/include/clang/Driver/Options.td

6880

You mean add DXCOption flag for _SLASH_I?
Is it OK to modify options for cl mode?

[beanz]

clang/include/clang/Driver/Options.td

6880

It is preferable to add DXCOption to existing options rather than declaring new options for everything.

[python3kgae]

Add CLDXCOption flag and share option with CL mode.

[beanz]

clang/include/clang/Driver/Options.td

6367

Do we need this group or can it just be cl_Group?

[python3kgae]

clang/include/clang/Driver/Options.td

6367

Yes. Without the group, it fails.
Also the text is keeping as cl_Group to avoid change help for cl mode.

[beanz]

This change works and avoids the duplicated group. The duplicated group results in the help spew duplicating the group heading, which is undesirable.

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 6dd4ec82d376..2dcdbae4385d 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -6310,10 +6310,7 @@ def defsym : Separate<["-"], "defsym">,
 // clang-cl Options
 //===----------------------------------------------------------------------===//
 
-def cl_Group : OptionGroup<"<clang-cl options>">, Flags<[CLOption]>,
-  HelpText<"CL.EXE COMPATIBILITY OPTIONS">;
-
-def cl_dxc_Group : OptionGroup<"<clang-cl options>">, Flags<[CLDXCOption]>,
+def cl_Group : OptionGroup<"<clang-cl options>">, Flags<[CLDXCOption]>,
   HelpText<"CL.EXE COMPATIBILITY OPTIONS">;
 
 def cl_compile_Group : OptionGroup<"<clang-cl compile-only options>">,
@@ -6344,7 +6341,7 @@ class CLJoinedOrSeparate<string name> : Option<["/", "-"], name,
   KIND_JOINED_OR_SEPARATE>, Group<cl_Group>, Flags<[CLOption, NoXarchOption]>;
 
 class CLDXCJoinedOrSeparate<string name> : Option<["/", "-"], name,
-  KIND_JOINED_OR_SEPARATE>, Group<cl_dxc_Group>, Flags<[CLDXCOption, NoXarchOption]>;
+  KIND_JOINED_OR_SEPARATE>, Group<cl_Group>, Flags<[CLDXCOption, NoXarchOption]>;
 
 class CLCompileJoinedOrSeparate<string name> : Option<["/", "-"], name,
   KIND_JOINED_OR_SEPARATE>, Group<cl_compile_Group>,
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index bc26f0d279a2..ee72f0fcb54c 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -6238,6 +6238,10 @@ Driver::getIncludeExcludeOptionFlagMasks(bool IsClCompatMode) const {
   } else {
     ExcludedFlagsBitmask |= options::DXCOption;
   }
+
+  if (!IsClCompatMode && !IsDXCMode())
+    ExcludedFlagsBitmask |= options::CLDXCOption;
+
   return std::make_pair(IncludedFlagsBitmask, ExcludedFlagsBitmask);
 }

[python3kgae]

Merge change from Chris.

[python3kgae]

Remove DXCJoinedOrSeparateConflict.

[python3kgae]

Fix test fail in clangd.

[beanz]

LGTM

[kadircet]

clang/include/clang/Driver/Options.td

6362

i am failing to understand the semantics of this new option group, and it's causing regressions in clangd. see https://github.com/clangd/clangd/issues/1292.

is this new group suppose to serve flags that are available both in --driver-mode=cl and --driver-mode=dxc? i.e. they are for the flags in the intersection?
are there any flags that are applicable to CL driver mode but not to the DXC mode (and vice versa)?

why didn't we go for just adding DXCOption mode into here, if every option in CL mode is actually applicable to DXC option as well, rather than introducing a new option?

[beanz]

clang/include/clang/Driver/Options.td

6362

DXC does not support all the CL options, and vice versa. There are some shared options, which is what this group tries to capture.

[kadircet]

clang/include/clang/Driver/Options.td

6362

okay thanks for the explanation. i'll fix it on the clangd side since the new option group seems to be necessary indeed (or we actually need to disentangle the include/exclude logic inside the driver).

it's still a little bit unfortunate that we actually need a new option flag just because we actually need to perform both inclusions and exclusions of options (i.e. we can't simply let this be CLOption | DXCOption, because being in --driver-mode=dxc would actually disable all options marked with CLOption).
This feels like a really fragile design and also implies such changes needs to take good care of all the call sites that does option parsing to make sure the newly introduced options are handled appropriately. I wonder why we can't just have a parser that requires inclusions. That way rather than introducing new option flags for such cases, we can just union them and users of the option parser would only lack the ability to parse new option groups, rather than having regressions around parsing existing options.

WDYT @jansvoboda11 @MaskRay (as owners for driver and compiler options) ? Is this something that has been on your radar?

[kadircet]

BTW, I am not sure when the new DXC driver mode was introduced but ATM it isn't properly handled by clang-tooling APIs, especially clangd. As the logic there wasn't updated accordingly, and it also resulted in regressions for clang-cl mode. I've sent out https://reviews.llvm.org/D133962 to remedy the regressions at least (which needs to be cherry-picked into next minor release I am afraid).
This is actually probably causing bigger troubles at the moment as a file that starts with /I will actually be treated as an include search path, rather than the input of the compilation, in places where CLDXC flags aren't excluded.

It would be nice to monitor new driver mode introductions to make sure they're not making the logic more complicated, as parsing needs to happen in other places as well (clangd, lld, lldb and a bunch of other tools parse compile flags themselves) and this logic gets duplicated into those places because driver doesn't have a public interface that exposes this (mostly because getIncludeExcludeOptionFlagMasks is complicated, and it's more complicated after this new driver mode).
Not supporting new driver modes in those tools, unless explicitly implemented, is probably OK, but it's really unfortunate that they'll break silently (due to changes similar to this). Because even if we were to test for all of the existing flags in those tools, we can't really test for possible flags that might be added in the future, which might all of a sudden change a filename to a command line option as seen here.

[beanz]

I think we had no expectation that DXC-mode would be supported by the tooling APIs at this point.

For context, DXC is the HLSL compiler that is based on clang-3.7. The DXC mode provides interface compatibility with it. DXC doesn't work with any of the clang tooling infrastructure, and clang's HLSL support isn't complete enough that people are going to be relying on tooling. I expect that we'll make larger investments in the Tooling library as we get further along in the HLSL implementation.

[kadircet]

In D128462#3793266, @beanz wrote:

I think we had no expectation that DXC-mode would be supported by the tooling APIs at this point.

Right, and as I mentioned in my previous comments this is completely fine, at least in the start. because as more and more developers start working with your toolchain, they'll also start using clang-tools and their bugs will be on us, rather than the maintainers of these toolchain/driver modes. which I believe puts the pressure on the wrong end, because we cannot even know what the intended way of handling these flags are, let alone knowing about the details of file discovery etc. that turns out to be a problem if not done properly.
My main complaint here's around breaking functionality for existing driver-modes/toolchains, and I am asking either getting OWNERS approvals going forward or being more through with the changes or changing the design overall to make these less complicated.

[beanz]

I understand your frustration with the regression, but let’s try to constructive. We all care about quality and we’re all working hard to do the best we can.

In D128462#3794868, @kadircet wrote:

My main complaint here's around breaking functionality for existing driver-modes/toolchains, and I am asking either getting OWNERS approvals going forward or being more through with the changes or changing the design overall to make these less complicated.

Regressions happen. There are things we can do to avoid them (code review being one), but the most fool-proof way to avoid regressions is test coverage. Here we had a gap in test coverage of existing functionality. A new change went in, all the tests passed, but an important usage regression occurred. It is unfortunate, but it happened.

Getting owner approval before commit has never been required by our developer policy. In fact, the wording of the policy puts the onus on the owner for post-commit review. In this case we had a fairly innocuous seeming change by a relatively new contributor and I reviewed and approved it because it seemed fairly innocuous. This said, hopefully the recent changes in Clang code ownership will help here too.

This change was tested, and introduces a new test in accordance with our community standards and policy.

I don’t think it is fair to say this change contributed to a complicated design other than that llvm’s libOption is pretty complicated and has lots of difficult to see edge cases, and the tools that share option parsing logic often have disparate and duplicated logic for handing those options which can cause a variety of issues.

In terms of your other complaints around bug handling; I also understand frustration there. Unfortunately, clangd has a separate bug reporting and tracking process from the rest of the LLVM and Clang code it depends on. I’m sure it is frustrating to get bug reports that relate to code you didn’t write or design, but I think that is a reality of the process and complexity of the software that we have today. It is incredibly common in LLVM for bugs to get routed to the wrong area and need to be rerouted. Sometimes this is because users reporting issues don’t understand the architecture of the code base well enough to route to the right place, other times it is because the cultural boundaries of responsibility in LLVM aren’t well codified.

In order to support the incremental implementation process that LLVM has always used we need a way to be able to add driver and language modes incrementally. If there’s a gap in the Tooling testing or architecture we can change to make that better, let’s please discuss that, rather than trying to apply additional hurdles to a review process because a regression was introduced.

clang-tools-extra/clangd/CompileCommands.cpp

unsigned char getModes(const llvm::opt::Option &Opt) {
   // Why is this so complicated?!
   // Reference is clang::driver::Driver::getIncludeExcludeOptionFlagMasks()
   unsigned char Result = DM_None;
   if (Opt.hasFlag(driver::options::CC1Option))
     Result |= DM_CC1;
   if (!Opt.hasFlag(driver::options::NoDriverOption)) {
     if (Opt.hasFlag(driver::options::CLOption)) {
       Result |= DM_CL;
+    } else if (Opt.hasFlag(driver::options::CLDXCOption)) {
+      Result |= DM_CL;
     } else {
       Result |= DM_GCC;
       if (Opt.hasFlag(driver::options::CoreOption)) {
         Result |= DM_CL;
       }
     }
   }
   return Result;

clang/include/clang/Driver/Options.h

enum ClangFlags {
   CC1Option = (1 << 10),
   CC1AsOption = (1 << 11),
   NoDriverOption = (1 << 12),
   LinkOption = (1 << 13),
   FlangOption = (1 << 14),
   FC1Option = (1 << 15),
   FlangOnlyOption = (1 << 16),
   DXCOption = (1 << 17),
-  Ignored = (1 << 18),
+  CLDXCOption = (1 << 18),
+  Ignored = (1 << 19),
 };
 
 enum ID {
     OPT_INVALID = 0, // This is not an option ID.
 #define OPTION(PREFIX, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM,  \
                HELPTEXT, METAVAR, VALUES)                                      \
   OPT_##ID,
 #include "clang/Driver/Options.inc"

clang/include/clang/Driver/Options.td

 
 // CC1AsOption - This option should be accepted by clang -cc1as.
 def CC1AsOption : OptionFlag;
 
 // DXCOption - This is a dxc.exe compatibility option. Options with this flag
 // are made available when the driver is running in DXC compatibility mode.
 def DXCOption : OptionFlag;
 
+// CLDXCOption - This is a cl.exe/dxc.exe compatibility option. Options with this flag
+// are made available when the driver is running in CL/DXC compatibility mode.
+def CLDXCOption : OptionFlag;
+
 // NoDriverOption - This option should not be accepted by the driver.
 def NoDriverOption : OptionFlag;
 
 // If an option affects linking, but has a primary group (so Link_Group cannot
 // be used), add this flag.
 def LinkOption : OptionFlag;
 
 // FlangOption - This is considered a "core" Flang option, available in
def defsym : Separate<["-"], "defsym">,
   HelpText<"Define a value for a symbol">;
 
 } // let Flags = [CC1AsOption]
 
 //===----------------------------------------------------------------------===//
 // clang-cl Options
 //===----------------------------------------------------------------------===//
 
-def cl_Group : OptionGroup<"<clang-cl options>">, Flags<[CLOption]>,
+def cl_Group : OptionGroup<"<clang-cl options>">, Flags<[CLDXCOption]>,
   HelpText<"CL.EXE COMPATIBILITY OPTIONS">;
 
 def cl_compile_Group : OptionGroup<"<clang-cl compile-only options>">,
   Group<cl_Group>;
 
 def cl_ignored_Group : OptionGroup<"<clang-cl ignored options>">,
   Group<cl_Group>;
 
 class CLFlag<string name> : Option<["/", "-"], name, KIND_FLAG>,
   Group<cl_Group>, Flags<[CLOption, NoXarchOption]>;
 
 class CLCompileFlag<string name> : Option<["/", "-"], name, KIND_FLAG>,
   Group<cl_compile_Group>, Flags<[CLOption, NoXarchOption]>;
 
 class CLIgnoredFlag<string name> : Option<["/", "-"], name, KIND_FLAG>,
   Group<cl_ignored_Group>, Flags<[CLOption, NoXarchOption]>;
 
 class CLJoined<string name> : Option<["/", "-"], name, KIND_JOINED>,
   Group<cl_Group>, Flags<[CLOption, NoXarchOption]>;
 
 class CLCompileJoined<string name> : Option<["/", "-"], name, KIND_JOINED>,
   Group<cl_compile_Group>, Flags<[CLOption, NoXarchOption]>;
 
 class CLIgnoredJoined<string name> : Option<["/", "-"], name, KIND_JOINED>,
   Group<cl_ignored_Group>, Flags<[CLOption, NoXarchOption, HelpHidden]>;
 
 class CLJoinedOrSeparate<string name> : Option<["/", "-"], name,
   KIND_JOINED_OR_SEPARATE>, Group<cl_Group>, Flags<[CLOption, NoXarchOption]>;
 
+class CLDXCJoinedOrSeparate<string name> : Option<["/", "-"], name,
+  KIND_JOINED_OR_SEPARATE>, Group<cl_Group>, Flags<[CLDXCOption, NoXarchOption]>;
+
 class CLCompileJoinedOrSeparate<string name> : Option<["/", "-"], name,
   KIND_JOINED_OR_SEPARATE>, Group<cl_compile_Group>,
   Flags<[CLOption, NoXarchOption]>;
 
 class CLRemainingArgsJoined<string name> : Option<["/", "-"], name,
   KIND_REMAINING_ARGS_JOINED>, Group<cl_Group>, Flags<[CLOption, NoXarchOption]>;
 
 // Aliases:

 def _SLASH_Gw_ : CLFlag<"Gw-">,
   HelpText<"Do not put each data item in its own section (default)">,
   Alias<fno_data_sections>;
 def _SLASH_help : CLFlag<"help">, Alias<help>,
   HelpText<"Display available options">;
 def _SLASH_HELP : CLFlag<"HELP">, Alias<help>;
 def _SLASH_hotpatch : CLFlag<"hotpatch">, Alias<fms_hotpatch>,
   HelpText<"Create hotpatchable image">;
-def _SLASH_I : CLJoinedOrSeparate<"I">,
+def _SLASH_I : CLDXCJoinedOrSeparate<"I">,
   HelpText<"Add directory to include search path">, MetaVarName<"<dir>">,
   Alias<I>;
 def _SLASH_J : CLFlag<"J">, HelpText<"Make char type unsigned">,
   Alias<funsigned_char>;
 
 // The _SLASH_O option handles all the /O flags, but we also provide separate
 // aliased options to provide separate help messages.
 def _SLASH_O : CLJoined<"O">,

 
 def dxc_Group : OptionGroup<"<clang-dxc options>">, Flags<[DXCOption]>,
   HelpText<"dxc compatibility options">;
 class DXCFlag<string name> : Option<["/", "-"], name, KIND_FLAG>,
   Group<dxc_Group>, Flags<[DXCOption, NoXarchOption]>;
 class DXCJoinedOrSeparate<string name> : Option<["/", "-"], name,
   KIND_JOINED_OR_SEPARATE>, Group<dxc_Group>, Flags<[DXCOption, NoXarchOption]>;
 
 def dxc_help : Option<["/", "-", "--"], "help", KIND_JOINED>,
   Group<dxc_Group>, Flags<[DXCOption, NoXarchOption]>, Alias<help>,
   HelpText<"Display available options">;
 def dxc_no_stdinc : DXCFlag<"hlsl-no-stdinc">,
   HelpText<"HLSL only. Disables all standard includes containing non-native compiler types and functions.">;
 def Fo : DXCJoinedOrSeparate<"Fo">, Alias<o>,
   HelpText<"Output object file">;
 def dxil_validator_version : Option<["/", "-"], "validator-version", KIND_SEPARATE>,
   Group<dxc_Group>, Flags<[DXCOption, NoXarchOption, CC1Option, HelpHidden]>,
   HelpText<"Override validator version for module. Format: <major.minor>;"
            "Default: DXIL.dll version or current internal version">,
   MarshallingInfoString<TargetOpts<"DxilValidatorVersion">>;
 def target_profile : DXCJoinedOrSeparate<"T">, MetaVarName<"<profile>">,
   HelpText<"Set target profile">,
   Values<"ps_6_0, ps_6_1, ps_6_2, ps_6_3, ps_6_4, ps_6_5, ps_6_6, ps_6_7,"
          "vs_6_0, vs_6_1, vs_6_2, vs_6_3, vs_6_4, vs_6_5, vs_6_6, vs_6_7,"
          "gs_6_0, gs_6_1, gs_6_2, gs_6_3, gs_6_4, gs_6_5, gs_6_6, gs_6_7,"
          "hs_6_0, hs_6_1, hs_6_2, hs_6_3, hs_6_4, hs_6_5, hs_6_6, hs_6_7,"
          "ds_6_0, ds_6_1, ds_6_2, ds_6_3, ds_6_4, ds_6_5, ds_6_6, ds_6_7,"
          "cs_6_0, cs_6_1, cs_6_2, cs_6_3, cs_6_4, cs_6_5, cs_6_6, cs_6_7,"
          "lib_6_3, lib_6_4, lib_6_5, lib_6_6, lib_6_7, lib_6_x,"
          "ms_6_5, ms_6_6, ms_6_7,"
          "as_6_5, as_6_6, as_6_7">;
 def dxc_D : Option<["--", "/", "-"], "D", KIND_JOINED_OR_SEPARATE>,
   Group<dxc_Group>, Flags<[DXCOption, NoXarchOption]>, Alias<D>;
 def emit_pristine_llvm : DXCFlag<"emit-pristine-llvm">,
   HelpText<"Emit pristine LLVM IR from the frontend by not running any LLVM passes at all."
            "Same as -S + -emit-llvm + -disable-llvm-passes.">;
 def fcgl : DXCFlag<"fcgl">, Alias<emit_pristine_llvm>;
 def enable_16bit_types : DXCFlag<"enable-16bit-types">, Alias<fnative_half_type>,
   HelpText<"Enable 16-bit types and disable min precision types."
            "Available in HLSL 2018 and shader model 6.2.">;

clang/lib/Driver/Driver.cpp

 std::pair<unsigned, unsigned>
 Driver::getIncludeExcludeOptionFlagMasks(bool IsClCompatMode) const {
   unsigned IncludedFlagsBitmask = 0;
   unsigned ExcludedFlagsBitmask = options::NoDriverOption;
 
   if (IsClCompatMode) {
     // Include CL and Core options.
     IncludedFlagsBitmask |= options::CLOption;
+    IncludedFlagsBitmask |= options::CLDXCOption;
     IncludedFlagsBitmask |= options::CoreOption;
   } else {
     ExcludedFlagsBitmask |= options::CLOption;
   }
   if (IsDXCMode()) {
     // Include DXC and Core options.
     IncludedFlagsBitmask |= options::DXCOption;
+    IncludedFlagsBitmask |= options::CLDXCOption;
     IncludedFlagsBitmask |= options::CoreOption;
   } else {
     ExcludedFlagsBitmask |= options::DXCOption;
   }
+  if (!IsClCompatMode && !IsDXCMode())
+    ExcludedFlagsBitmask |= options::CLDXCOption;
+
   return std::make_pair(IncludedFlagsBitmask, ExcludedFlagsBitmask);
 }
 
 bool clang::driver::isOptimizationLevelFast(const ArgList &Args) {
   return Args.hasFlag(options::OPT_Ofast, options::OPT_O_Group, false);
 }
 
 bool clang::driver::willEmitRemarks(const ArgList &Args) {

clang/lib/Driver/ToolChains/Clang.cpp

if ((types::isOpenCL(InputType) ||
     CmdArgs.push_back("-fdeclare-opencl-builtins");
   }
 }
 
 static void RenderHLSLOptions(const ArgList &Args, ArgStringList &CmdArgs,
                               types::ID InputType) {
   const unsigned ForwardedArguments[] = {options::OPT_dxil_validator_version,
                                          options::OPT_D,
+                                         options::OPT_I,
                                          options::OPT_S,
                                          options::OPT_emit_llvm,
                                          options::OPT_disable_llvm_passes,
                                          options::OPT_fnative_half_type};
 
   for (const auto &Arg : ForwardedArguments)
     if (const auto *A = Args.getLastArg(Arg))
       A->renderAsInput(Args, CmdArgs);

clang/test/Driver/dxc_I.hlsl

+// RUN: %clang_dxc -I test  -### %s 2>&1 | FileCheck %s
+
+// Make sure -I send to cc1.
+// CHECK:"-I" "test"