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

Loop Counter Increment in Collective Pipeliner #19363

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

philipphack
Copy link
Contributor

Sets the loop iteration counter increment in the backward transformation of the collective pipeliner pass to account for cases with non-zero initial value of the loop iteration counter. See #16953 and #18568.

@yliu120
Copy link
Contributor

yliu120 commented Nov 15, 2024

Thanks. I guess this PR fixes the CP bug.

But there is another bug that causes corrupted numerics when the stack layer is unrolled. @hawkinsp

@abhinavgoel95
Copy link

@yliu120 what is the other bug? Is it related to this PR?

@yliu120
Copy link
Contributor

yliu120 commented Nov 15, 2024

@yliu120 what is the other bug? Is it related to this PR?

Unrelated. This PR fixed the issue in the CP pass and please go ahead!

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.

Is it possible to reproduce the numerical issue in collective_ops_e2e_test.cc and write a test there to make sure it does not regress?

[](const HloInstruction* instruction) {
return HloPredicateIsOp<HloOpcode::kWhile>(instruction);
});
EXPECT_EQ(while_count, 1);
Copy link
Member

Choose a reason for hiding this comment

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

Can you rewrite this part to filecheck? All you want to check is the increment so it should not be long, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed some of the verifications duplicated in other tests but didn't switch to FileCheck since e.g. TransformIncrementIndexByOneBackwards doesn't use it either. Let me know if you want to use it anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Without so many assertions I think in practice it's now less mandated :)

@hawkinsp
Copy link
Member

@philipphack Any chance you can address Greg's comments soon? I'd like to get this PR into the next release of JAX (tomorrow, hopefully).

@philipphack
Copy link
Contributor Author

I added a set of collective pipeliner tests to collective_ops_e2e_test.cc.

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.

Thanks for adding e2e test!

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

Sets the loop iteration counter increment in the backward transformation of the collective pipeliner pass to account for cases with non-zero initial value of the loop iteration counter. See #16953 and #18568.
Copybara import of the project:

--
06137aa by Philipp Hack <[email protected]>:

Modifies the loop counter increment set in the backward transformation of the collective pipeliner.

--
6da45bc by Philipp Hack <[email protected]>:

Modifies the loop counter increment set in the backward transformation of the collective pipeliner.

Merging this change closes #19363

FUTURE_COPYBARA_INTEGRATE_REVIEW=#19363 from philipphack:u_pipeliner_increment_xla 6da45bc
PiperOrigin-RevId: 698294264
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Nov 20, 2024
Imported from GitHub PR openxla/xla#19363

Sets the loop iteration counter increment in the backward transformation of the collective pipeliner pass to account for cases with non-zero initial value of the loop iteration counter. See #16953 and #18568.
Copybara import of the project:

--
06137aa0618d372e2d4badbf16920bead9922cfb by Philipp Hack <[email protected]>:

Modifies the loop counter increment set in the backward transformation of the collective pipeliner.

--
6da45bcb26643d8994bf608f05230fa748286b02 by Philipp Hack <[email protected]>:

Modifies the loop counter increment set in the backward transformation of the collective pipeliner.

Merging this change closes #19363

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#19363 from philipphack:u_pipeliner_increment_xla 6da45bcb26643d8994bf608f05230fa748286b02
PiperOrigin-RevId: 698294264
copybara-service bot pushed a commit that referenced this pull request Nov 20, 2024
Imported from GitHub PR #19363

Sets the loop iteration counter increment in the backward transformation of the collective pipeliner pass to account for cases with non-zero initial value of the loop iteration counter. See #16953 and #18568.
Copybara import of the project:

--
06137aa by Philipp Hack <[email protected]>:

Modifies the loop counter increment set in the backward transformation of the collective pipeliner.

--
6da45bc by Philipp Hack <[email protected]>:

Modifies the loop counter increment set in the backward transformation of the collective pipeliner.

Merging this change closes #19363

FUTURE_COPYBARA_INTEGRATE_REVIEW=#19363 from philipphack:u_pipeliner_increment_xla 6da45bc
PiperOrigin-RevId: 698294264
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Nov 20, 2024
Imported from GitHub PR openxla/xla#19363

Sets the loop iteration counter increment in the backward transformation of the collective pipeliner pass to account for cases with non-zero initial value of the loop iteration counter. See #16953 and #18568.
Copybara import of the project:

--
06137aa0618d372e2d4badbf16920bead9922cfb by Philipp Hack <[email protected]>:

Modifies the loop counter increment set in the backward transformation of the collective pipeliner.

--
6da45bcb26643d8994bf608f05230fa748286b02 by Philipp Hack <[email protected]>:

Modifies the loop counter increment set in the backward transformation of the collective pipeliner.

Merging this change closes #19363

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#19363 from philipphack:u_pipeliner_increment_xla 6da45bcb26643d8994bf608f05230fa748286b02
PiperOrigin-RevId: 698294264
copybara-service bot pushed a commit that referenced this pull request Nov 20, 2024
Imported from GitHub PR #19363

Sets the loop iteration counter increment in the backward transformation of the collective pipeliner pass to account for cases with non-zero initial value of the loop iteration counter. See #16953 and #18568.
Copybara import of the project:

--
06137aa by Philipp Hack <[email protected]>:

Modifies the loop counter increment set in the backward transformation of the collective pipeliner.

--
6da45bc by Philipp Hack <[email protected]>:

Modifies the loop counter increment set in the backward transformation of the collective pipeliner.

Merging this change closes #19363

FUTURE_COPYBARA_INTEGRATE_REVIEW=#19363 from philipphack:u_pipeliner_increment_xla 6da45bc
PiperOrigin-RevId: 698294264
copybara-service bot pushed a commit to tensorflow/tensorflow that referenced this pull request Nov 20, 2024
Imported from GitHub PR openxla/xla#19363

Sets the loop iteration counter increment in the backward transformation of the collective pipeliner pass to account for cases with non-zero initial value of the loop iteration counter. See #16953 and #18568.
Copybara import of the project:

--
06137aa0618d372e2d4badbf16920bead9922cfb by Philipp Hack <[email protected]>:

Modifies the loop counter increment set in the backward transformation of the collective pipeliner.

--
6da45bcb26643d8994bf608f05230fa748286b02 by Philipp Hack <[email protected]>:

Modifies the loop counter increment set in the backward transformation of the collective pipeliner.

Merging this change closes #19363

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#19363 from philipphack:u_pipeliner_increment_xla 6da45bcb26643d8994bf608f05230fa748286b02
PiperOrigin-RevId: 698294264
copybara-service bot pushed a commit that referenced this pull request Nov 20, 2024
Imported from GitHub PR #19363

Sets the loop iteration counter increment in the backward transformation of the collective pipeliner pass to account for cases with non-zero initial value of the loop iteration counter. See #16953 and #18568.
Copybara import of the project:

--
06137aa by Philipp Hack <[email protected]>:

Modifies the loop counter increment set in the backward transformation of the collective pipeliner.

--
6da45bc by Philipp Hack <[email protected]>:

Modifies the loop counter increment set in the backward transformation of the collective pipeliner.

Merging this change closes #19363

FUTURE_COPYBARA_INTEGRATE_REVIEW=#19363 from philipphack:u_pipeliner_increment_xla 6da45bc
PiperOrigin-RevId: 698294264
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