Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable resource directory for DXC. #6954

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

s-perron
Copy link
Collaborator

This change enables the resource directory for DXC. This will allow the HLSL standard headers to be accessed without using the -I option.

Copy link
Contributor

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff b26fd8099aa435c6a46b10b179549280ba3930b0 a6ebd017c51e5687fd040aaa7a8623c9ee57417f -- tools/clang/lib/Frontend/InitHeaderSearch.cpp tools/clang/tools/dxcompiler/dxcfilesystem.cpp tools/clang/tools/dxcompiler/dxcompilerobj.cpp
View the diff from clang-format here.
diff --git a/tools/clang/lib/Frontend/InitHeaderSearch.cpp b/tools/clang/lib/Frontend/InitHeaderSearch.cpp
index 38612398..fc5627f1 100644
--- a/tools/clang/lib/Frontend/InitHeaderSearch.cpp
+++ b/tools/clang/lib/Frontend/InitHeaderSearch.cpp
@@ -442,12 +442,12 @@ void InitHeaderSearch::AddDefaultIncludePaths(const LangOptions &Lang,
                                               const llvm::Triple &triple,
                                             const HeaderSearchOptions &HSOpts) {
 #if 1 // HLSL Change Starts
-    if (HSOpts.UseBuiltinIncludes) {
-        SmallString<128> P = StringRef(HSOpts.ResourceDir);
-        llvm::sys::path::append(P, "include");
-        AddUnmappedPath(P, Angled, false);
-    }
-    return;
+  if (HSOpts.UseBuiltinIncludes) {
+    SmallString<128> P = StringRef(HSOpts.ResourceDir);
+    llvm::sys::path::append(P, "include");
+    AddUnmappedPath(P, Angled, false);
+  }
+  return;
 #else
   // NB: This code path is going away. All of the logic is moving into the
   // driver which has the information necessary to do target-specific
diff --git a/tools/clang/tools/dxcompiler/dxcfilesystem.cpp b/tools/clang/tools/dxcompiler/dxcfilesystem.cpp
index 19209df0..82633f08 100644
--- a/tools/clang/tools/dxcompiler/dxcfilesystem.cpp
+++ b/tools/clang/tools/dxcompiler/dxcfilesystem.cpp
@@ -397,9 +397,10 @@ public:
       }
     }
     if (compiler.getHeaderSearchOpts().UseBuiltinIncludes) {
-        SmallString<128> P = StringRef(compiler.getHeaderSearchOpts().ResourceDir);
-        llvm::sys::path::append(P, "include");
-        m_searchEntries.emplace_back(Unicode::UTF8ToWideStringOrThrow(P.c_str()));
+      SmallString<128> P =
+          StringRef(compiler.getHeaderSearchOpts().ResourceDir);
+      llvm::sys::path::append(P, "include");
+      m_searchEntries.emplace_back(Unicode::UTF8ToWideStringOrThrow(P.c_str()));
     }
   }
 
diff --git a/tools/clang/tools/dxcompiler/dxcompilerobj.cpp b/tools/clang/tools/dxcompiler/dxcompilerobj.cpp
index 95d2827a..398c91b1 100644
--- a/tools/clang/tools/dxcompiler/dxcompilerobj.cpp
+++ b/tools/clang/tools/dxcompiler/dxcompilerobj.cpp
@@ -1405,8 +1405,8 @@ public:
     // Pick additional arguments.
     clang::HeaderSearchOptions &HSOpts = compiler.getHeaderSearchOpts();
     HSOpts.UseBuiltinIncludes = true;
-    HSOpts.ResourceDir =
-      clang::CompilerInvocation::GetResourcesPath("Hoping this is not needed on any of our platforms", nullptr);
+    HSOpts.ResourceDir = clang::CompilerInvocation::GetResourcesPath(
+        "Hoping this is not needed on any of our platforms", nullptr);
     // Consider: should we force-include '.' if the source file is relative?
     for (const llvm::opt::Arg *A : Opts.Args.filtered(options::OPT_I)) {
       const bool IsFrameworkFalse = false;
  • Check this box to apply formatting changes to this branch.

HSOpts.UseBuiltinIncludes = 0;
HSOpts.UseBuiltinIncludes = true;
HSOpts.ResourceDir =
clang::CompilerInvocation::GetResourcesPath("Hoping this is not needed on any of our platforms", nullptr);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have something that "works", but there is a problem. I don't know the right way to pass new information from dxc::main to this point. I don't have the path for the executable from argv[0] available. It is skipped when passing along the argument.

This works on my linux system where GetResourcePath uses /proc/self/exe to get the executable, but I don't know how to set up these parameters to work on other platforms.

Note that we could call GetResourcePath in another place if we determine how to pass the inforamtion along. This just seemed like the best place for now because this is where we are processing the -I options.

@s-perron
Copy link
Collaborator Author

Things that still need to be done:

  • Modify the cmake install to copy the header to the resource directory.
  • Change the default path to something that makes sense for dxc.
  • Manually test on a few different platforms to make sure it works.

@s-perron
Copy link
Collaborator Author

@pow2clk @llvm-beanz

Do you have any suggestions on how to implement this? I'm not sure how to proceed. I don't know how to pass information from the driver into the library without changing the public interface.

@s-perron s-perron self-assigned this Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New
Development

Successfully merging this pull request may close these issues.

1 participant