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

Optionally don't inline stream annotated kCalls #18448

Conversation

chaserileyroberts
Copy link
Contributor

First part of splitting up #17982

Copy link
Member

@golechwierowicz golechwierowicz left a comment

Choose a reason for hiding this comment

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

Thank you for the PR split! Much appreciated!

xla/service/call_inliner_test.cc Outdated Show resolved Hide resolved
@chaserileyroberts chaserileyroberts force-pushed the chase/stream_call_noinline branch 2 times, most recently from 78cf7aa to 5ac671a Compare October 24, 2024 17:36
@chaserileyroberts
Copy link
Contributor Author

I've updated the test.

Copy link
Contributor

@sanjoy sanjoy left a comment

Choose a reason for hiding this comment

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

Left some minor nitpicky comments inline.

xla/service/call_inliner.cc Outdated Show resolved Hide resolved
xla/xla.proto Outdated Show resolved Hide resolved
)");
TF_ASSERT_OK(filecheck_result.status());
EXPECT_TRUE(*filecheck_result);

Copy link
Member

Choose a reason for hiding this comment

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

For the future: I think everything below that can be deleted, because it's already tested via filecheck.

Copy link
Member

Choose a reason for hiding this comment

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

+1

copybara-service bot pushed a commit that referenced this pull request Oct 29, 2024
Imported from GitHub PR #18448

First part of splitting up #17982
Copybara import of the project:

--
8ecc06c by chaser <[email protected]>:

Don't inline stream annotated kCalls

Merging this change closes #18448

FUTURE_COPYBARA_INTEGRATE_REVIEW=#18448 from chaserileyroberts:chase/stream_call_noinline 8ecc06c
PiperOrigin-RevId: 691070675
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Oct 29, 2024
Imported from GitHub PR openxla/xla#18448

First part of splitting up #17982
Copybara import of the project:

--
8ecc06c64422558094c5d1a913d4b425ca453435 by chaser <[email protected]>:

Don't inline stream annotated kCalls

Merging this change closes #18448

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#18448 from chaserileyroberts:chase/stream_call_noinline 8ecc06c64422558094c5d1a913d4b425ca453435
PiperOrigin-RevId: 691070675
)");
TF_ASSERT_OK(filecheck_result.status());
EXPECT_TRUE(*filecheck_result);

Copy link
Member

Choose a reason for hiding this comment

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

+1

copybara-service bot pushed a commit that referenced this pull request Nov 1, 2024
Imported from GitHub PR #18448

First part of splitting up #17982
Copybara import of the project:

--
8ecc06c by chaser <[email protected]>:

Don't inline stream annotated kCalls

Merging this change closes #18448

FUTURE_COPYBARA_INTEGRATE_REVIEW=#18448 from chaserileyroberts:chase/stream_call_noinline 8ecc06c
PiperOrigin-RevId: 691070675
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Nov 1, 2024
Imported from GitHub PR openxla/xla#18448

First part of splitting up #17982
Copybara import of the project:

--
8ecc06c64422558094c5d1a913d4b425ca453435 by chaser <[email protected]>:

Don't inline stream annotated kCalls

Merging this change closes #18448

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#18448 from chaserileyroberts:chase/stream_call_noinline 8ecc06c64422558094c5d1a913d4b425ca453435
PiperOrigin-RevId: 691070675
@loislo loislo self-requested a review November 4, 2024 13:35
copybara-service bot pushed a commit that referenced this pull request Nov 6, 2024
Imported from GitHub PR #18448

First part of splitting up #17982
Copybara import of the project:

--
8ecc06c by chaser <[email protected]>:

Don't inline stream annotated kCalls

Merging this change closes #18448

FUTURE_COPYBARA_INTEGRATE_REVIEW=#18448 from chaserileyroberts:chase/stream_call_noinline 8ecc06c
PiperOrigin-RevId: 691070675
copybara-service bot pushed a commit that referenced this pull request Nov 7, 2024
Imported from GitHub PR #18448

First part of splitting up #17982
Copybara import of the project:

--
8ecc06c by chaser <[email protected]>:

Don't inline stream annotated kCalls

Merging this change closes #18448

FUTURE_COPYBARA_INTEGRATE_REVIEW=#18448 from chaserileyroberts:chase/stream_call_noinline 8ecc06c
PiperOrigin-RevId: 691070675
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Nov 7, 2024
Imported from GitHub PR openxla/xla#18448

First part of splitting up #17982
Copybara import of the project:

--
8ecc06c64422558094c5d1a913d4b425ca453435 by chaser <[email protected]>:

Don't inline stream annotated kCalls

Merging this change closes #18448

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#18448 from chaserileyroberts:chase/stream_call_noinline 8ecc06c64422558094c5d1a913d4b425ca453435
PiperOrigin-RevId: 691070675
copybara-service bot pushed a commit that referenced this pull request Nov 7, 2024
Imported from GitHub PR #18448

First part of splitting up #17982
Copybara import of the project:

--
8ecc06c by chaser <[email protected]>:

Don't inline stream annotated kCalls

Merging this change closes #18448

FUTURE_COPYBARA_INTEGRATE_REVIEW=#18448 from chaserileyroberts:chase/stream_call_noinline 8ecc06c
PiperOrigin-RevId: 691070675
copybara-service bot pushed a commit that referenced this pull request Nov 7, 2024
Imported from GitHub PR #18448

First part of splitting up #17982
Copybara import of the project:

--
8ecc06c by chaser <[email protected]>:

Don't inline stream annotated kCalls

Merging this change closes #18448

FUTURE_COPYBARA_INTEGRATE_REVIEW=#18448 from chaserileyroberts:chase/stream_call_noinline 8ecc06c
PiperOrigin-RevId: 691070675
copybara-service bot pushed a commit that referenced this pull request Nov 7, 2024
Imported from GitHub PR #18448

First part of splitting up #17982
Copybara import of the project:

--
8ecc06c by chaser <[email protected]>:

Don't inline stream annotated kCalls

Merging this change closes #18448

FUTURE_COPYBARA_INTEGRATE_REVIEW=#18448 from chaserileyroberts:chase/stream_call_noinline 8ecc06c
PiperOrigin-RevId: 691070675
copybara-service bot pushed a commit that referenced this pull request Nov 7, 2024
Imported from GitHub PR #18448

First part of splitting up #17982
Copybara import of the project:

--
8ecc06c by chaser <[email protected]>:

Don't inline stream annotated kCalls

Merging this change closes #18448

FUTURE_COPYBARA_INTEGRATE_REVIEW=#18448 from chaserileyroberts:chase/stream_call_noinline 8ecc06c
PiperOrigin-RevId: 691070675
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Nov 7, 2024
Imported from GitHub PR openxla/xla#18448

First part of splitting up #17982
Copybara import of the project:

--
8ecc06c64422558094c5d1a913d4b425ca453435 by chaser <[email protected]>:

Don't inline stream annotated kCalls

Merging this change closes #18448

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#18448 from chaserileyroberts:chase/stream_call_noinline 8ecc06c64422558094c5d1a913d4b425ca453435
PiperOrigin-RevId: 691070675
copybara-service bot pushed a commit that referenced this pull request Nov 8, 2024
Imported from GitHub PR #18448

First part of splitting up #17982
Copybara import of the project:

--
8ecc06c by chaser <[email protected]>:

Don't inline stream annotated kCalls

Merging this change closes #18448

FUTURE_COPYBARA_INTEGRATE_REVIEW=#18448 from chaserileyroberts:chase/stream_call_noinline 8ecc06c
PiperOrigin-RevId: 691070675
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Nov 8, 2024
Imported from GitHub PR openxla/xla#18448

First part of splitting up #17982
Copybara import of the project:

--
8ecc06c64422558094c5d1a913d4b425ca453435 by chaser <[email protected]>:

Don't inline stream annotated kCalls

Merging this change closes #18448

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#18448 from chaserileyroberts:chase/stream_call_noinline 8ecc06c64422558094c5d1a913d4b425ca453435
PiperOrigin-RevId: 691070675
@copybara-service copybara-service bot closed this in 151c19d Nov 8, 2024
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Nov 8, 2024
Imported from GitHub PR openxla/xla#18448

First part of splitting up #17982
Copybara import of the project:

--
8ecc06c64422558094c5d1a913d4b425ca453435 by chaser <[email protected]>:

Don't inline stream annotated kCalls

Merging this change closes #18448

PiperOrigin-RevId: 694522066
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Nov 8, 2024
TritonSupportTest relies on a death test for some of its assertion because Triton doesn't allow graceful error handling for some of the cases we want to test.

This death test also used to succeed when the code under test triggered a santizer violation.

So this change makes it fail on those sanitizer violations and will be surface those in the log.

Ideally we would tightly control in which code path each death test terminates, but unfortunately most of them don't have indicative error message. Some just call `std::abort` without an error message. Some just die in accessing an empty `std::optional`, etc. So the best we can do is make sure that we detect sanitizer errors and report these as test failures.

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#18448 from chaserileyroberts:chase/stream_call_noinline 8ecc06c64422558094c5d1a913d4b425ca453435
PiperOrigin-RevId: 688604240
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.

4 participants