[HLSL][clang][Driver] Parse target profile early to update Driver::TargetTriple.

[python3kgae]

Summary

This is to avoid err_target_unknown_abi which is caused by use default TargetTriple instead of shader model target triple.

[beanz]

clang/lib/Driver/ToolChains/HLSL.h

32–33

I don't like using an empty string as a sentinel for an error. Since this is no longer overriding an existing method, we should change it. Probably the easiest change is to llvm::Optional. Then you can return None in the failure cases.

[python3kgae]

Use llvm::Optional.

[beanz]

clang/lib/Driver/Driver.cpp

1242

The -T flag is required for DXCMode, so the else of this should emit an error. That also means the code above that explicitly sets the OS to ShaderModel can be removed too.

[python3kgae]

Add check for no -T option.

[beanz]

LGTM.

[vitalybuka]

@python3kgae
0fbafb5a1c4381ded4bc7f59a5a6091c229faed7 is fixing leaks in this tests.

Note, the leak was missed because of unrelated issues in gtest and lsan setup which we fixed recently.

I incorrectly blamed the leak on D122865, but in fact it was D125585. I see that before this patch we have one Driver, one Compilation and both deleted correctly. Leaks are obvious, after this patch we don't delete Compilation.

I fixed the leak, but I had to remove shared driver. My concern is that maybe shared driver is important use-case. If it's so, would you like to take a look, and find how to avoid a leak with shared driver?

[python3kgae]

In D125585#3657734, @vitalybuka wrote:

@python3kgae
0fbafb5a1c4381ded4bc7f59a5a6091c229faed7 is fixing leaks in this tests.

Note, the leak was missed because of unrelated issues in gtest and lsan setup which we fixed recently.

I incorrectly blamed the leak on D122865, but in fact it was D125585. I see that before this patch we have one Driver, one Compilation and both deleted correctly. Leaks are obvious, after this patch we don't delete Compilation.

I fixed the leak, but I had to remove shared driver. My concern is that maybe shared driver is important use-case. If it's so, would you like to take a look, and find how to avoid a leak with shared driver?

Hi Vitaly,
Thanks for fixing the leak. Shared driver is not important use-case.

clang/include/clang/Basic/DiagnosticDriverKinds.td

def warn_drv_fjmc_for_elf_only : Warning<
   "-fjmc works only for ELF; option ignored">,
   InGroup<OptionIgnored>;
 
 def err_drv_target_variant_invalid : Error<
   "unsupported '%0' value '%1'; use 'ios-macabi' instead">;
 
 def err_drv_invalid_directx_shader_module : Error<
   "invalid profile : %0">;
+def err_drv_dxc_missing_target_profile : Error<
+  "target profile option (-T) is missing">;
 
 def err_drv_invalid_range_dxil_validator_version : Error<
   "invalid validator version : %0\n"
   "Validator version must be less than or equal to current internal version.">;
 def err_drv_invalid_format_dxil_validator_version : Error<
   "invalid validator version : %0\n"
   "Format of validator version is \"<major>.<minor>\" (ex:\"1.4\").">;
 def err_drv_invalid_empty_dxil_validator_version : Error<
   "invalid validator version : %0\n"
   "If validator major version is 0, minor version must also be 0.">;
 }

clang/lib/Driver/Driver.cpp

if (IsCLMode()) {
     // clang-cl targets MSVC-style Win32.
     llvm::Triple T(TargetTriple);
     T.setOS(llvm::Triple::Win32);
     T.setVendor(llvm::Triple::PC);
     T.setEnvironment(llvm::Triple::MSVC);
     T.setObjectFormat(llvm::Triple::COFF);
     TargetTriple = T.str();
   } else if (IsDXCMode()) {
-    // clang-dxc target is build from target_profile option.
-    // Just set OS to shader model to select HLSLToolChain.
-    llvm::Triple T(TargetTriple);
-    T.setOS(llvm::Triple::ShaderModel);
-    TargetTriple = T.str();
+    // Build TargetTriple from target_profile option for clang-dxc.
+    if (const Arg *A = Args.getLastArg(options::OPT_target_profile)) {
+      StringRef TargetProfile = A->getValue();
+      if (auto Triple =
+              toolchains::HLSLToolChain::parseTargetProfile(TargetProfile))
+        TargetTriple = Triple.getValue();
+      else
+        Diag(diag::err_drv_invalid_directx_shader_module) << TargetProfile;
+
+      A->claim();
+    } else {
+      Diag(diag::err_drv_dxc_missing_target_profile);
+    }
   }
 
   if (const Arg *A = Args.getLastArg(options::OPT_target))
     TargetTriple = A->getValue();
   if (const Arg *A = Args.getLastArg(options::OPT_ccc_install_dir))
     Dir = InstalledDir = A->getValue();
   for (const Arg *A : Args.filtered(options::OPT_B)) {
     A->claim();

clang/lib/Driver/ToolChains/HLSL.h

public:
   bool isPIEDefault(const llvm::opt::ArgList &Args) const override {
     return false;
   }
   bool isPICDefaultForced() const override { return false; }
 
   llvm::opt::DerivedArgList *
   TranslateArgs(const llvm::opt::DerivedArgList &Args, StringRef BoundArch,
                 Action::OffloadKind DeviceOffloadKind) const override;
-  std::string ComputeEffectiveClangTriple(const llvm::opt::ArgList &Args,
-                                          types::ID InputType) const override;
+  static llvm::Optional<std::string>
+  parseTargetProfile(StringRef TargetProfile);
 };
 
 } // end namespace toolchains
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_HLSL_H

clang/lib/Driver/ToolChains/HLSL.cpp

bool isLegalShaderModel(Triple &T) {
   case Triple::EnvironmentType::Mesh: {
     VersionTuple MinVer(6, 5);
     return MinVer <= Version;
   } break;
   }
   return false;
 }
 
-std::string tryParseProfile(StringRef Profile) {
+llvm::Optional<std::string> tryParseProfile(StringRef Profile) {
   // [ps|vs|gs|hs|ds|cs|ms|as]_[major]_[minor]
   SmallVector<StringRef, 3> Parts;
   Profile.split(Parts, "_");
   if (Parts.size() != 3)
-    return "";
+    return NoneType();
 
   Triple::EnvironmentType Kind =
       StringSwitch<Triple::EnvironmentType>(Parts[0])
           .Case("ps", Triple::EnvironmentType::Pixel)
           .Case("vs", Triple::EnvironmentType::Vertex)
           .Case("gs", Triple::EnvironmentType::Geometry)
           .Case("hs", Triple::EnvironmentType::Hull)
           .Case("ds", Triple::EnvironmentType::Domain)
           .Case("cs", Triple::EnvironmentType::Compute)
           .Case("lib", Triple::EnvironmentType::Library)
           .Case("ms", Triple::EnvironmentType::Mesh)
           .Case("as", Triple::EnvironmentType::Amplification)
           .Default(Triple::EnvironmentType::UnknownEnvironment);
   if (Kind == Triple::EnvironmentType::UnknownEnvironment)
-    return "";
+    return NoneType();
 
   unsigned long long Major = 0;
   if (llvm::getAsUnsignedInteger(Parts[1], 0, Major))
-    return "";
+    return NoneType();
 
   unsigned long long Minor = 0;
   if (Parts[2] == "x" && Kind == Triple::EnvironmentType::Library)
     Minor = OfflineLibMinor;
   else if (llvm::getAsUnsignedInteger(Parts[2], 0, Minor))
-    return "";
+    return NoneType();
 
   // dxil-unknown-shadermodel-hull
   llvm::Triple T;
   T.setArch(Triple::ArchType::dxil);
   T.setOSName(Triple::getOSTypeName(Triple::OSType::ShaderModel).str() +
               VersionTuple(Major, Minor).getAsString());
   T.setEnvironment(Kind);
   if (isLegalShaderModel(T))
     return T.getTriple();
   else
-    return "";
+    return NoneType();
 }
 
 bool isLegalValidatorVersion(StringRef ValVersionStr, const Driver &D) {
   VersionTuple Version;
   if (Version.tryParse(ValVersionStr) || Version.getBuild() ||
       Version.getSubminor() || !Version.getMinor()) {
     D.Diag(diag::err_drv_invalid_format_dxil_validator_version)
         << ValVersionStr;

 
 } // namespace
 
 /// DirectX Toolchain
 HLSLToolChain::HLSLToolChain(const Driver &D, const llvm::Triple &Triple,
                              const ArgList &Args)
     : ToolChain(D, Triple, Args) {}
 
-std::string
-HLSLToolChain::ComputeEffectiveClangTriple(const ArgList &Args,
-                                           types::ID InputType) const {
-  if (Arg *A = Args.getLastArg(options::OPT_target_profile)) {
-    StringRef Profile = A->getValue();
-    std::string Triple = tryParseProfile(Profile);
-    if (Triple == "") {
-      getDriver().Diag(diag::err_drv_invalid_directx_shader_module) << Profile;
-      Triple = ToolChain::ComputeEffectiveClangTriple(Args, InputType);
-    }
-    A->claim();
-    return Triple;
-  } else {
-    return ToolChain::ComputeEffectiveClangTriple(Args, InputType);
-  }
+llvm::Optional<std::string>
+clang::driver::toolchains::HLSLToolChain::parseTargetProfile(
+    StringRef TargetProfile) {
+  return tryParseProfile(TargetProfile);
 }
 
 DerivedArgList *
 HLSLToolChain::TranslateArgs(const DerivedArgList &Args, StringRef BoundArch,
                              Action::OffloadKind DeviceOffloadKind) const {
   DerivedArgList *DAL = new DerivedArgList(Args.getBaseArgs());
 
   const OptTable &Opts = getDriver().getOpts();

clang/test/Driver/dxc_fcgl.hlsl

-// RUN: %clang_dxc -fcgl foo.hlsl -### %s 2>&1 | FileCheck %s
+// RUN: %clang_dxc -fcgl -T lib_6_7 foo.hlsl -### %s 2>&1 | FileCheck %s
 
 // Make sure fcgl option flag which translated into "-S" "-emit-llvm" "-disable-llvm-passes".
 // CHECK:"-S" "-emit-llvm" "-disable-llvm-passes"
 

clang/unittests/Driver/ToolChainTest.cpp

void clear() override {
     Msgs.clear();
     Errors.clear();
     DiagnosticConsumer::clear();
   }
   std::vector<SmallString<32>> Msgs;
   std::vector<SmallString<32>> Errors;
 };
 
+static void validateTargetProfile(StringRef TargetProfile,
+                                  StringRef ExpectTriple, Driver &TheDriver,
+                                  DiagnosticsEngine &Diags) {
+  EXPECT_TRUE(TheDriver.BuildCompilation(
+      {"clang", "--driver-mode=dxc", TargetProfile.data(), "foo.hlsl"}));
+  EXPECT_STREQ(TheDriver.getTargetTriple().c_str(), ExpectTriple.data());
+  EXPECT_EQ(Diags.getNumErrors(), 0u);
+}
+
+static void validateTargetProfile(StringRef TargetProfile,
+                                  StringRef ExpectError, Driver &TheDriver,
+                                  DiagnosticsEngine &Diags,
+                                  SimpleDiagnosticConsumer *DiagConsumer,
+                                  unsigned NumOfErrors) {
+  EXPECT_TRUE(TheDriver.BuildCompilation(
+      {"clang", "--driver-mode=dxc", TargetProfile.data(), "foo.hlsl"}));
+  EXPECT_EQ(Diags.getNumErrors(), NumOfErrors);
+  EXPECT_STREQ(DiagConsumer->Errors.back().c_str(), ExpectError.data());
+  Diags.Clear();
+  DiagConsumer->clear();
+}
+
 TEST(DxcModeTest, TargetProfileValidation) {
   IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
 
   IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFileSystem(
       new llvm::vfs::InMemoryFileSystem);
 
   InMemoryFileSystem->addFile("foo.hlsl", 0,
                               llvm::MemoryBuffer::getMemBuffer("\n"));
 
   auto *DiagConsumer = new SimpleDiagnosticConsumer;
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
   DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagConsumer);
   Driver TheDriver("/bin/clang", "", Diags, "", InMemoryFileSystem);
-  std::unique_ptr<Compilation> C(
-      TheDriver.BuildCompilation({"clang", "--driver-mode=dxc", "foo.hlsl"}));
-  EXPECT_TRUE(C);
-  EXPECT_TRUE(!C->containsError());
-
-  auto &TC = C->getDefaultToolChain();
-  bool ContainsError = false;
-  auto Args = TheDriver.ParseArgStrings({"-Tvs_6_0"}, false, ContainsError);
-  EXPECT_FALSE(ContainsError);
-  auto Triple = TC.ComputeEffectiveClangTriple(Args);
-  EXPECT_STREQ(Triple.c_str(), "dxil--shadermodel6.0-vertex");
-  EXPECT_EQ(Diags.getNumErrors(), 0u);
 
-  Args = TheDriver.ParseArgStrings({"-Ths_6_1"}, false, ContainsError);
-  EXPECT_FALSE(ContainsError);
-  Triple = TC.ComputeEffectiveClangTriple(Args);
-  EXPECT_STREQ(Triple.c_str(), "dxil--shadermodel6.1-hull");
-  EXPECT_EQ(Diags.getNumErrors(), 0u);
-
-  Args = TheDriver.ParseArgStrings({"-Tds_6_2"}, false, ContainsError);
-  EXPECT_FALSE(ContainsError);
-  Triple = TC.ComputeEffectiveClangTriple(Args);
-  EXPECT_STREQ(Triple.c_str(), "dxil--shadermodel6.2-domain");
-  EXPECT_EQ(Diags.getNumErrors(), 0u);
-
-  Args = TheDriver.ParseArgStrings({"-Tds_6_2"}, false, ContainsError);
-  EXPECT_FALSE(ContainsError);
-  Triple = TC.ComputeEffectiveClangTriple(Args);
-  EXPECT_STREQ(Triple.c_str(), "dxil--shadermodel6.2-domain");
-  EXPECT_EQ(Diags.getNumErrors(), 0u);
-
-  Args = TheDriver.ParseArgStrings({"-Tgs_6_3"}, false, ContainsError);
-  EXPECT_FALSE(ContainsError);
-  Triple = TC.ComputeEffectiveClangTriple(Args);
-  EXPECT_STREQ(Triple.c_str(), "dxil--shadermodel6.3-geometry");
-  EXPECT_EQ(Diags.getNumErrors(), 0u);
-
-  Args = TheDriver.ParseArgStrings({"-Tps_6_4"}, false, ContainsError);
-  EXPECT_FALSE(ContainsError);
-  Triple = TC.ComputeEffectiveClangTriple(Args);
-  EXPECT_STREQ(Triple.c_str(), "dxil--shadermodel6.4-pixel");
-  EXPECT_EQ(Diags.getNumErrors(), 0u);
-
-  Args = TheDriver.ParseArgStrings({"-Tcs_6_5"}, false, ContainsError);
-  EXPECT_FALSE(ContainsError);
-  Triple = TC.ComputeEffectiveClangTriple(Args);
-  EXPECT_STREQ(Triple.c_str(), "dxil--shadermodel6.5-compute");
-  EXPECT_EQ(Diags.getNumErrors(), 0u);
-
-  Args = TheDriver.ParseArgStrings({"-Tms_6_6"}, false, ContainsError);
-  EXPECT_FALSE(ContainsError);
-  Triple = TC.ComputeEffectiveClangTriple(Args);
-  EXPECT_STREQ(Triple.c_str(), "dxil--shadermodel6.6-mesh");
-  EXPECT_EQ(Diags.getNumErrors(), 0u);
-
-  Args = TheDriver.ParseArgStrings({"-Tas_6_7"}, false, ContainsError);
-  EXPECT_FALSE(ContainsError);
-  Triple = TC.ComputeEffectiveClangTriple(Args);
-  EXPECT_STREQ(Triple.c_str(), "dxil--shadermodel6.7-amplification");
-  EXPECT_EQ(Diags.getNumErrors(), 0u);
-
-  Args = TheDriver.ParseArgStrings({"-Tlib_6_x"}, false, ContainsError);
-  EXPECT_FALSE(ContainsError);
-  Triple = TC.ComputeEffectiveClangTriple(Args);
-  EXPECT_STREQ(Triple.c_str(), "dxil--shadermodel6.15-library");
-  EXPECT_EQ(Diags.getNumErrors(), 0u);
+  validateTargetProfile("-Tvs_6_0", "dxil--shadermodel6.0-vertex", TheDriver,
+                        Diags);
+  validateTargetProfile("-Ths_6_1", "dxil--shadermodel6.1-hull", TheDriver,
+                        Diags);
+  validateTargetProfile("-Tds_6_2", "dxil--shadermodel6.2-domain", TheDriver,
+                        Diags);
+  validateTargetProfile("-Tds_6_2", "dxil--shadermodel6.2-domain", TheDriver,
+                        Diags);
+  validateTargetProfile("-Tgs_6_3", "dxil--shadermodel6.3-geometry", TheDriver,
+                        Diags);
+  validateTargetProfile("-Tps_6_4", "dxil--shadermodel6.4-pixel", TheDriver,
+                        Diags);
+  validateTargetProfile("-Tcs_6_5", "dxil--shadermodel6.5-compute", TheDriver,
+                        Diags);
+  validateTargetProfile("-Tms_6_6", "dxil--shadermodel6.6-mesh", TheDriver,
+                        Diags);
+  validateTargetProfile("-Tas_6_7", "dxil--shadermodel6.7-amplification",
+                        TheDriver, Diags);
+  validateTargetProfile("-Tlib_6_x", "dxil--shadermodel6.15-library", TheDriver,
+                        Diags);
 
   // Invalid tests.
-  Args = TheDriver.ParseArgStrings({"-Tpss_6_1"}, false, ContainsError);
-  EXPECT_FALSE(ContainsError);
-  Triple = TC.ComputeEffectiveClangTriple(Args);
-  EXPECT_STREQ(Triple.c_str(), "unknown-unknown-shadermodel");
-  EXPECT_EQ(Diags.getNumErrors(), 1u);
-  EXPECT_STREQ(DiagConsumer->Errors.back().c_str(),
-               "invalid profile : pss_6_1");
-  Diags.Clear();
-  DiagConsumer->clear();
-
-  Args = TheDriver.ParseArgStrings({"-Tps_6_x"}, false, ContainsError);
-  EXPECT_FALSE(ContainsError);
-  Triple = TC.ComputeEffectiveClangTriple(Args);
-  EXPECT_STREQ(Triple.c_str(), "unknown-unknown-shadermodel");
-  EXPECT_EQ(Diags.getNumErrors(), 2u);
-  EXPECT_STREQ(DiagConsumer->Errors.back().c_str(), "invalid profile : ps_6_x");
-  Diags.Clear();
-  DiagConsumer->clear();
+  validateTargetProfile("-Tpss_6_1", "invalid profile : pss_6_1", TheDriver,
+                        Diags, DiagConsumer, 1);
 
-  Args = TheDriver.ParseArgStrings({"-Tlib_6_1"}, false, ContainsError);
-  EXPECT_FALSE(ContainsError);
-  Triple = TC.ComputeEffectiveClangTriple(Args);
-  EXPECT_STREQ(Triple.c_str(), "unknown-unknown-shadermodel");
-  EXPECT_EQ(Diags.getNumErrors(), 3u);
-  EXPECT_STREQ(DiagConsumer->Errors.back().c_str(),
-               "invalid profile : lib_6_1");
-  Diags.Clear();
-  DiagConsumer->clear();
-
-  Args = TheDriver.ParseArgStrings({"-Tfoo"}, false, ContainsError);
-  EXPECT_FALSE(ContainsError);
-  Triple = TC.ComputeEffectiveClangTriple(Args);
-  EXPECT_STREQ(Triple.c_str(), "unknown-unknown-shadermodel");
-  EXPECT_EQ(Diags.getNumErrors(), 4u);
-  EXPECT_STREQ(DiagConsumer->Errors.back().c_str(), "invalid profile : foo");
-  Diags.Clear();
-  DiagConsumer->clear();
+  validateTargetProfile("-Tps_6_x", "invalid profile : ps_6_x", TheDriver,
+                        Diags, DiagConsumer, 2);
+  validateTargetProfile("-Tlib_6_1", "invalid profile : lib_6_1", TheDriver,
+                        Diags, DiagConsumer, 3);
+  validateTargetProfile("-Tfoo", "invalid profile : foo", TheDriver, Diags,
+                        DiagConsumer, 4);
+  validateTargetProfile("", "target profile option (-T) is missing", TheDriver,
+                        Diags, DiagConsumer, 5);
 }
 
 TEST(DxcModeTest, ValidatorVersionValidation) {
   IntrusiveRefCntPtr<DiagnosticIDs> DiagID(new DiagnosticIDs());
 
   IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFileSystem(
       new llvm::vfs::InMemoryFileSystem);
 
   InMemoryFileSystem->addFile("foo.hlsl", 0,
                               llvm::MemoryBuffer::getMemBuffer("\n"));
 
   auto *DiagConsumer = new SimpleDiagnosticConsumer;
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
   DiagnosticsEngine Diags(DiagID, &*DiagOpts, DiagConsumer);
   Driver TheDriver("/bin/clang", "", Diags, "", InMemoryFileSystem);
-  std::unique_ptr<Compilation> C(
-      TheDriver.BuildCompilation({"clang", "--driver-mode=dxc", "foo.hlsl"}));
+  std::unique_ptr<Compilation> C(TheDriver.BuildCompilation(
+      {"clang", "--driver-mode=dxc", "-Tlib_6_7", "foo.hlsl"}));
   EXPECT_TRUE(C);
   EXPECT_TRUE(!C->containsError());
 
   auto &TC = C->getDefaultToolChain();
   bool ContainsError = false;
   auto Args = TheDriver.ParseArgStrings({"-validator-version", "1.1"}, false,
                                         ContainsError);
   EXPECT_FALSE(ContainsError);