[SYCL] Don't print the header and footer in the preprocessed output. #15634

zahiraam · 2024-10-08T16:45:53Z

This is needed to support compilation with ccache.
When compiling with ccache, the integration header and footer shouldn't be printed in the pre-processed output to not confuse ccache. In order to do that we added a new option called include-internal-footer to include the integration footer. For consistency the include option, that was including the integration header was renamed -include-internal-header.

clang/lib/Driver/ToolChains/Clang.cpp

mdtoguchi · 2024-10-14T16:46:32Z

clang/test/Driver/sycl-int-header.cpp

@@ -0,0 +1,86 @@
+/// Check compilation tool steps when using the integration footer


Test name is sycl-int-header.cpp but comments in the test are checking for the footer, but the tests themselves are checking for both. Can this ben cleaned up?

asudarsa · 2024-10-14T17:11:18Z

clang/lib/Driver/ToolChains/Clang.cpp

      }

-      // Add the -include-footer option to add the integration footer
+      // Add the -include-internal-footer option to add the integration footer


Nit: Add the comment inside the 'if' construct so that it reads similar to the
'header' option?

asudarsa · 2024-10-14T17:58:54Z

llvm/include/llvm/Support/Path.h

+///
+/// @param path Input path.
+/// @result The cleaned-up \a path.
+StringRef remove_leading_dotbackslah(StringRef path);


Suggested change

StringRef remove_leading_dotbackslah(StringRef path);

StringRef remove_leading_dotbackslash(StringRef path);

asudarsa · 2024-10-14T17:59:07Z

llvm/lib/Support/Path.cpp

  return Path;
 }

+StringRef remove_leading_dotbackslah(StringRef Path) {


Suggested change

StringRef remove_leading_dotbackslah(StringRef Path) {

StringRef remove_leading_dotbackslash(StringRef Path) {

asudarsa · 2024-10-14T18:00:15Z

clang/lib/Frontend/PrintPreprocessedOutput.cpp

-    OS->write_escaped(CurFilename);
-    *OS << '"';
+  StringRef CurFilenameWithNoLeaningDotSlash =
+      llvm::sys::path::remove_leading_dotbackslah(CurFilename.str());


Suggested change

llvm::sys::path::remove_leading_dotbackslah(CurFilename.str());

llvm::sys::path::remove_leading_dotbackslash(CurFilename.str());

asudarsa · 2024-10-14T18:00:51Z

clang/lib/Frontend/PrintPreprocessedOutput.cpp

                                             unsigned ExtraLen) {
  startNewLineIfNeeded();

  // Emit #line directives or GNU line markers depending on what mode we're in.


May be comment should be moved a bit down?

asudarsa · 2024-10-14T18:02:58Z

clang/include/clang/Driver/Options.td

    MetaVarName<"<file>">, HelpText<"Include file before parsing">,
    Visibility<[ClangOption, CC1Option]>;
-def include_footer : Separate<["-"], "include-footer">, Group<clang_i_Group>,
+def include_internal_footer : Separate<["-"], "include-internal-footer">, Group<clang_i_Group>,


Is there a possibility where we want to include header and not footer...or the other way around? Just wondering if we need two options here.

Thanks

Good question. @premanandrao What do you think?

The behavior of -include-internal-header and -include-internal-footer is more than just the ability to pass in a file to be included, it also designates where in the preprocessing the file is expanded, so we need to have 2 options.

Thanks Mike!

asudarsa · 2024-10-14T18:03:15Z

clang/lib/Driver/ToolChains/Clang.cpp

      if (types::getPreprocessedType(Input.getType()) != types::TY_INVALID &&
          !Args.hasArg(options::OPT_fno_sycl_use_footer) && !Footer.empty()) {
-        CmdArgs.push_back("-include-footer");
+        // Add the -include-internal-footer option to add the integration foote


Suggested change

// Add the -include-internal-footer option to add the integration foote

// Add the -include-internal-footer option to add the integration footer

asudarsa · 2024-10-14T18:04:24Z

clang/lib/Frontend/InitPreprocessor.cpp

    AddImplicitInclude(Builder, Path);
  }

+  // Process -include-internal-header directive.


Is this comment accurate? Is the 'processing' happening inside the function call?

Yes. A #include header.h is added to the stream.

asudarsa · 2024-10-14T18:05:12Z

clang/lib/Frontend/PrintPreprocessedOutput.cpp

+  if (value == '\\')
+    return true;
+  return false;


Suggested change

if (value == '\\')

return true;

return false;

return value == '\\';

asudarsa

Minor nits. Overall looks good. Thanks

asudarsa

LGTM. Thanks

zahiraam · 2024-10-16T16:56:13Z

@intel/dpcpp-cfe-reviewers can you please review this? Thanks.

elizabethandrews

@premanandrao can you please take a look at this? I believe you reviewed previous versions of this functionality.

premanandrao · 2024-10-18T15:48:39Z

clang/lib/Frontend/PrintPreprocessedOutput.cpp

+  StringRef CurFilenameWithNoLeaningDotSlash =
+      llvm::sys::path::remove_leading_dotbackslash(CurFilename.str());
+  if ((CurFilenameWithNoLeaningDotSlash ==
+              PP.getPreprocessorOpts().IncludeFooter) ||
+      CurFilenameWithNoLeaningDotSlash ==


Suggested change

StringRef CurFilenameWithNoLeaningDotSlash =

llvm::sys::path::remove_leading_dotbackslash(CurFilename.str());

if ((CurFilenameWithNoLeaningDotSlash ==

PP.getPreprocessorOpts().IncludeFooter) ||

CurFilenameWithNoLeaningDotSlash ==

StringRef CurFilenameWithNoLeadingDotSlash =

llvm::sys::path::remove_leading_dotbackslash(CurFilename.str());

if ((CurFilenameWithNoLeadingDotSlash ==

PP.getPreprocessorOpts().IncludeFooter) ||

CurFilenameWithNoLeadingDotSlash ==

premanandrao · 2024-10-18T15:50:13Z

clang/lib/Frontend/PrintPreprocessedOutput.cpp

+              PP.getPreprocessorOpts().IncludeHeader) {
+    CurFilename = "<uninit>";
+  }
+    // Emit #line directives or GNU line markers depending on what mode we're in.


The indentation starting this line seems off to me. Please check.

premanandrao · 2024-10-18T16:01:35Z

clang/test/Preprocessor/print_line_empty_file.c

 int bar() { return 21; }

-// CHECK: # 21 ""
+// CHECK: # 1 "[[INPUT:.+\.c]]" 2


What is this line for?

This is for the empty line. Fixed that.

premanandrao · 2024-10-18T16:08:14Z

llvm/lib/Support/Path.cpp

+    Path = Path.substr(2);
+  return Path;
+}
+


Instead of a new routine, can remove_leading_dotslash be reused with the appropriate Style?

It's not the Style argument that's the issue, it's the call to is_separator that would make the change more complicated than it should be. Furthermore, we only need to check if we have the .\\ at the beginning of the string, no need to visit the entire string which is what remove_leading_dotslash is doing.
But I can make the change if you don't want to see this additional function.

I see. You specifically don't want the loop? In that case, I am fine with your new function - except maybe give it a distinct name, so the leading vs all distinction is clear.

How about something like: remove_leading_dotbackslash_only? Just a suggestion. Please pick a spelling that sounds good to you.

Sure. I can name it that way. Thanks.

premanandrao

Just one minor nit. Other than that, looks good.

premanandrao · 2024-10-22T20:20:22Z

clang/lib/Frontend/PrintPreprocessedOutput.cpp

  startNewLineIfNeeded();

+  if (PP.getLangOpts().isSYCL()) {
+  StringRef CurFilenameWithNoLeadingDotSlash =


Indentation.

zahiraam · 2024-10-23T11:13:37Z

@intel/llvm-gatekeepers Can this be merged in please? Thanks.

zahiraam and others added 7 commits September 18, 2024 05:44

[SYCL] Don't print the header and footer in the preprocessed output.

d8b91f9

Fix for LIT fails.

a87ef5d

Fixed conflict.

bf359e9

Merge branch 'sycl' into CcacheOffload

e4d04d1

Fix LIT failures.

19806c1

Fix LIT test.

6eadb5a

Clean code.

eda1496

zahiraam had a problem deploying to WindowsCILock October 8, 2024 16:47 — with GitHub Actions Error

Changed the CurFilename to uninit.

10d54cc

zahiraam temporarily deployed to WindowsCILock October 8, 2024 18:15 — with GitHub Actions Inactive

zahiraam temporarily deployed to WindowsCILock October 9, 2024 20:01 — with GitHub Actions Inactive

mdtoguchi reviewed Oct 10, 2024

View reviewed changes

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

Addressed review comments and refactored the code.

09f735e

zahiraam temporarily deployed to WindowsCILock October 11, 2024 15:20 — with GitHub Actions Inactive

zahiraam changed the title ~~Save temps~~ [SYCL] Don't print the header and footer in the preprocessed output. Oct 11, 2024

zahiraam temporarily deployed to WindowsCILock October 11, 2024 17:33 — with GitHub Actions Inactive

zahiraam marked this pull request as ready for review October 14, 2024 08:08

zahiraam requested review from a team as code owners October 14, 2024 08:08

mdtoguchi reviewed Oct 14, 2024

View reviewed changes

asudarsa reviewed Oct 14, 2024

View reviewed changes

Fix LIT test.

82c3d86

zahiraam had a problem deploying to WindowsCILock October 14, 2024 17:13 — with GitHub Actions Error

Fix comment.

be076b4

zahiraam temporarily deployed to WindowsCILock October 14, 2024 17:18 — with GitHub Actions Inactive

asudarsa reviewed Oct 14, 2024

View reviewed changes

zahiraam temporarily deployed to WindowsCILock October 14, 2024 18:45 — with GitHub Actions Inactive

Addressed review comments.

83794f6

zahiraam temporarily deployed to WindowsCILock October 14, 2024 19:41 — with GitHub Actions Inactive

asudarsa approved these changes Oct 14, 2024

View reviewed changes

zahiraam temporarily deployed to WindowsCILock October 14, 2024 21:28 — with GitHub Actions Inactive

mdtoguchi approved these changes Oct 15, 2024

View reviewed changes

elizabethandrews reviewed Oct 17, 2024

View reviewed changes

premanandrao reviewed Oct 18, 2024

View reviewed changes

Addressed review comments.

7fe537c

zahiraam temporarily deployed to WindowsCILock October 21, 2024 12:56 — with GitHub Actions Inactive

zahiraam had a problem deploying to WindowsCILock October 21, 2024 14:28 — with GitHub Actions Failure

zahiraam had a problem deploying to WindowsCILock October 21, 2024 15:33 — with GitHub Actions Error

Addressed review comments.

8d48644

zahiraam temporarily deployed to WindowsCILock October 21, 2024 16:12 — with GitHub Actions Inactive

zahiraam temporarily deployed to WindowsCILock October 21, 2024 17:58 — with GitHub Actions Inactive

zahiraam requested a review from premanandrao October 21, 2024 21:27

premanandrao approved these changes Oct 22, 2024

View reviewed changes

Fix format.

e408c27

zahiraam temporarily deployed to WindowsCILock October 22, 2024 21:31 — with GitHub Actions Inactive

zahiraam temporarily deployed to WindowsCILock October 23, 2024 00:04 — with GitHub Actions Inactive

martygrant merged commit 4c47642 into intel:sycl Oct 23, 2024
11 of 12 checks passed

mdtoguchi mentioned this pull request Nov 19, 2024

[SYCL] ccache is invalidated by random file names generated for offloaded code bundles #5260

Closed

		@@ -0,0 +1,86 @@
		/// Check compilation tool steps when using the integration footer

	StringRef remove_leading_dotbackslah(StringRef path);
	StringRef remove_leading_dotbackslash(StringRef path);

	llvm::sys::path::remove_leading_dotbackslah(CurFilename.str());
	llvm::sys::path::remove_leading_dotbackslash(CurFilename.str());

	// Add the -include-internal-footer option to add the integration foote
	// Add the -include-internal-footer option to add the integration footer

[SYCL] Don't print the header and footer in the preprocessed output. #15634

[SYCL] Don't print the header and footer in the preprocessed output. #15634

Uh oh!

Conversation

zahiraam commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asudarsa left a comment

Choose a reason for hiding this comment

Uh oh!

asudarsa left a comment

Choose a reason for hiding this comment

Uh oh!

zahiraam commented Oct 16, 2024

Uh oh!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zahiraam commented Oct 23, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zahiraam commented Oct 8, 2024 •

edited

Loading