-
Notifications
You must be signed in to change notification settings - Fork 126
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Implement ConditionallySpeculatable for collective ops (#2168)
I'm actually not sure if speculation make sense for these ops. I don't see any indication in the spec that they could have UB except in the cases added here, so it's likely fine. Also, I'm not sure if these ops can have memory effects or not. They definitely involve interacting with some kind of global state, so they have side effects. The ops weren't marked "Pure" before this change, so there is no difference on that front being introduced with this change. Also I slightly updated the logic in TestUtils to delete the op when the speculation check succeeds. Indeed before we were relying on DCE, but these ops don't state that they don't have side effects so DCE won't remove them.
- Loading branch information
mlevesquedion
authored
Apr 9, 2024
1 parent
2651907
commit ad2a3b3
Showing
4 changed files
with
338 additions
and
6 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.