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

Fix missing template value #20340

Closed
wants to merge 5 commits into from

Conversation

charleshofer
Copy link
Contributor

Fixes a bug introduced in this change: google/tsl#2944

The change makes use of a template variable %{compiler}, that is not defined for this file. This causes the -fno-canonical-system-headers option to be set for Clang builds, and Clang will fail with an error about that command line flag not being defined.

Copy link
Contributor

@alekstheod alekstheod left a comment

Choose a reason for hiding this comment

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

lgtm

mmakevic-amd added a commit to ROCm/tensorflow-upstream that referenced this pull request Dec 11, 2024
@charleshofer
Copy link
Contributor Author

@xla-rotation Could I get a review on this? Right now this is breaking clang builds on ROCm JAX

@@ -792,6 +792,7 @@ def _create_local_rocm_repository(repository_ctx):
tpl_paths["crosstool:clang/bin/crosstool_wrapper_driver_rocm"],
{
"%{cpu_compiler}": str(cc),
"%{compiler}": "clang" if is_rocm_clang else "unknown",
Copy link
Member

Choose a reason for hiding this comment

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

Having a cpu_compiler and a compiler variable sounds confusing to me. What about rather introducing a new variable compiler_is_clang and fixing up the template file accordingly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds reasonable to me. I'll make that change.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

@charleshofer
Copy link
Contributor Author

Hey, how do I get this merged? This is breaking ROCm builds for JAX right now, and it's been sitting here for a few weeks.

@charleshofer charleshofer force-pushed the fix-missing-template-value branch from d7affaf to 373f359 Compare January 7, 2025 19:27
@hsharsha
Copy link
Contributor

hsharsha commented Jan 8, 2025

@xla-rotation there was a merge conflict which is resolved now. Please merge this change.

@hsharsha hsharsha force-pushed the fix-missing-template-value branch 2 times, most recently from 29c8761 to eafb70b Compare January 8, 2025 22:43
@hsharsha
Copy link
Contributor

hsharsha commented Jan 8, 2025

@beckerhe and @ezhulenev Can you please merge this change into main?

@hsharsha hsharsha force-pushed the fix-missing-template-value branch from eafb70b to d45a080 Compare January 8, 2025 22:56
copybara-service bot pushed a commit that referenced this pull request Jan 9, 2025
Imported from GitHub PR #20340

Fixes a bug introduced in this change: google/tsl#2944

The change makes use of a template variable `%{compiler}`, that is not defined for this file. This causes the `-fno-canonical-system-headers` option to be set for Clang builds, and Clang will fail with an error about that command line flag not being defined.
Copybara import of the project:

--
75a3d3f by Charles Hofer <[email protected]>:

Fix missing template value

--
e08537b by Charles Hofer <[email protected]>:

Change flag to compiler_is_clang

--
373f359 by Charles Hofer <[email protected]>:

Fix typo

--
2be3c30 by Harsha HS <[email protected]>:

[ROCm] Add cuda-only tags for nvidia profiler test

Merging this change closes #20340

FUTURE_COPYBARA_INTEGRATE_REVIEW=#20340 from ROCm:fix-missing-template-value 2be3c30
PiperOrigin-RevId: 713547337
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Jan 9, 2025
Imported from GitHub PR openxla/xla#20340

Fixes a bug introduced in this change: google/tsl#2944

The change makes use of a template variable `%{compiler}`, that is not defined for this file. This causes the `-fno-canonical-system-headers` option to be set for Clang builds, and Clang will fail with an error about that command line flag not being defined.
Copybara import of the project:

--
75a3d3fbcf2ead55df3872aa80ff21ac3dd9336c by Charles Hofer <[email protected]>:

Fix missing template value

--
e08537b09200b0037db7a05780dea0d525399376 by Charles Hofer <[email protected]>:

Change flag to compiler_is_clang

--
373f359cbd8d02ee850d98fed92a7bbca4a09c1b by Charles Hofer <[email protected]>:

Fix typo

--
2be3c309d05f93a48dd9fdd06af8159108920516 by Harsha HS <[email protected]>:

[ROCm] Add cuda-only tags for nvidia profiler test

Merging this change closes #20340

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#20340 from ROCm:fix-missing-template-value 2be3c309d05f93a48dd9fdd06af8159108920516
PiperOrigin-RevId: 713547337
copybara-service bot pushed a commit to google/tsl that referenced this pull request Jan 11, 2025
Imported from GitHub PR openxla/xla#20340

Fixes a bug introduced in this change: #2944

The change makes use of a template variable `%{compiler}`, that is not defined for this file. This causes the `-fno-canonical-system-headers` option to be set for Clang builds, and Clang will fail with an error about that command line flag not being defined.
Copybara import of the project:

--
75a3d3fbcf2ead55df3872aa80ff21ac3dd9336c by Charles Hofer <[email protected]>:

Fix missing template value

--
e08537b09200b0037db7a05780dea0d525399376 by Charles Hofer <[email protected]>:

Change flag to compiler_is_clang

--
373f359cbd8d02ee850d98fed92a7bbca4a09c1b by Charles Hofer <[email protected]>:

Fix typo

--
2be3c309d05f93a48dd9fdd06af8159108920516 by Harsha HS <[email protected]>:

[ROCm] Add cuda-only tags for nvidia profiler test

Merging this change closes #20340

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#20340 from ROCm:fix-missing-template-value 2be3c309d05f93a48dd9fdd06af8159108920516
PiperOrigin-RevId: 714282939
copybara-service bot pushed a commit that referenced this pull request Jan 11, 2025
Imported from GitHub PR #20340

Fixes a bug introduced in this change: google/tsl#2944

The change makes use of a template variable `%{compiler}`, that is not defined for this file. This causes the `-fno-canonical-system-headers` option to be set for Clang builds, and Clang will fail with an error about that command line flag not being defined.
Copybara import of the project:

--
75a3d3f by Charles Hofer <[email protected]>:

Fix missing template value

--
e08537b by Charles Hofer <[email protected]>:

Change flag to compiler_is_clang

--
373f359 by Charles Hofer <[email protected]>:

Fix typo

--
2be3c30 by Harsha HS <[email protected]>:

[ROCm] Add cuda-only tags for nvidia profiler test

Merging this change closes #20340

FUTURE_COPYBARA_INTEGRATE_REVIEW=#20340 from ROCm:fix-missing-template-value 2be3c30
PiperOrigin-RevId: 714282939
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Jan 11, 2025
Imported from GitHub PR openxla/xla#20340

Fixes a bug introduced in this change: google/tsl#2944

The change makes use of a template variable `%{compiler}`, that is not defined for this file. This causes the `-fno-canonical-system-headers` option to be set for Clang builds, and Clang will fail with an error about that command line flag not being defined.
Copybara import of the project:

--
75a3d3fbcf2ead55df3872aa80ff21ac3dd9336c by Charles Hofer <[email protected]>:

Fix missing template value

--
e08537b09200b0037db7a05780dea0d525399376 by Charles Hofer <[email protected]>:

Change flag to compiler_is_clang

--
373f359cbd8d02ee850d98fed92a7bbca4a09c1b by Charles Hofer <[email protected]>:

Fix typo

--
2be3c309d05f93a48dd9fdd06af8159108920516 by Harsha HS <[email protected]>:

[ROCm] Add cuda-only tags for nvidia profiler test

Merging this change closes #20340

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#20340 from ROCm:fix-missing-template-value 2be3c309d05f93a48dd9fdd06af8159108920516
PiperOrigin-RevId: 714282939
copybara-service bot pushed a commit to google/tsl that referenced this pull request Jan 11, 2025
Imported from GitHub PR openxla/xla#20340

Fixes a bug introduced in this change: #2944

The change makes use of a template variable `%{compiler}`, that is not defined for this file. This causes the `-fno-canonical-system-headers` option to be set for Clang builds, and Clang will fail with an error about that command line flag not being defined.
Copybara import of the project:

--
75a3d3fbcf2ead55df3872aa80ff21ac3dd9336c by Charles Hofer <[email protected]>:

Fix missing template value

--
e08537b09200b0037db7a05780dea0d525399376 by Charles Hofer <[email protected]>:

Change flag to compiler_is_clang

--
373f359cbd8d02ee850d98fed92a7bbca4a09c1b by Charles Hofer <[email protected]>:

Fix typo

--
2be3c309d05f93a48dd9fdd06af8159108920516 by Harsha HS <[email protected]>:

[ROCm] Add cuda-only tags for nvidia profiler test

Merging this change closes #20340

PiperOrigin-RevId: 714293326
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Jan 11, 2025
Imported from GitHub PR openxla/xla#20340

Fixes a bug introduced in this change: google/tsl#2944

The change makes use of a template variable `%{compiler}`, that is not defined for this file. This causes the `-fno-canonical-system-headers` option to be set for Clang builds, and Clang will fail with an error about that command line flag not being defined.
Copybara import of the project:

--
75a3d3fbcf2ead55df3872aa80ff21ac3dd9336c by Charles Hofer <[email protected]>:

Fix missing template value

--
e08537b09200b0037db7a05780dea0d525399376 by Charles Hofer <[email protected]>:

Change flag to compiler_is_clang

--
373f359cbd8d02ee850d98fed92a7bbca4a09c1b by Charles Hofer <[email protected]>:

Fix typo

--
2be3c309d05f93a48dd9fdd06af8159108920516 by Harsha HS <[email protected]>:

[ROCm] Add cuda-only tags for nvidia profiler test

Merging this change closes #20340

PiperOrigin-RevId: 714293326
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants