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

[stdlib] Mark assume as has_side_effect=False #3658

Closed
wants to merge 2 commits into from

Conversation

soraros
Copy link
Contributor

@soraros soraros commented Oct 13, 2024

Also mark assume as side-effect free.

@soraros soraros requested a review from a team as a code owner October 13, 2024 15:05
@soraros soraros force-pushed the debug-assert-assume branch 2 times, most recently from 72f50e1 to f929462 Compare October 13, 2024 15:29
@soraros
Copy link
Contributor Author

soraros commented Oct 13, 2024

Actually, will assume get DCE'd if we set has_side_effect to False?

@soraros soraros marked this pull request as draft October 13, 2024 19:28
@soraros soraros force-pushed the debug-assert-assume branch from f929462 to 92f1cae Compare October 14, 2024 08:00
@soraros soraros marked this pull request as ready for review October 14, 2024 19:09
@soraros soraros force-pushed the debug-assert-assume branch from 92f1cae to 9228148 Compare October 14, 2024 21:15
@JoeLoser
Copy link
Collaborator

@jackos do you mind taking a look at this? This is modeled after https://en.cppreference.com/w/cpp/language/attributes/assume.

@JoeLoser
Copy link
Collaborator

!sync

@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label Oct 16, 2024
@soraros soraros marked this pull request as draft October 16, 2024 14:58
@soraros
Copy link
Contributor Author

soraros commented Oct 16, 2024

Also mark assume as side-effect free.

@JoeLoser I now think this is the wrong thing to do. llvm.assume returns void, so if it's marked as pure, and nobody is using the result, it won't pass DCE. Nothing shows in the IR generated by ff.

fn ff(n: Int) -> Int:
  llvm_intrinsic["llvm.assume", NoneType, has_side_effect=False](n == 1)
  return 666 if n == 1 else 777

fn gg(n: Int) -> Int:
  llvm_intrinsic["llvm.assume", NoneType](n == 1)
  return 666 if n == 1 else 777

Maybe we still want to put assume in debug_assert, but we will have to wait for #3662.

@JoeLoser JoeLoser added the blocked Blocked by another issue that must be resolved first label Oct 16, 2024
@soraros soraros force-pushed the debug-assert-assume branch from 9228148 to 628430f Compare October 22, 2024 12:57
@jackos jackos marked this pull request as ready for review January 9, 2025 16:18
@jackos jackos force-pushed the debug-assert-assume branch from 628430f to 4ebb977 Compare January 9, 2025 16:23
@jackos jackos changed the title [stdlib] Make debug_assert generate llvm.assume when ASSERT=none [stdlib] Mark assume as has_side_effect=False Jan 9, 2025
Copy link
Collaborator

@jackos jackos left a comment

Choose a reason for hiding this comment

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

Hi @soraros I've updated externally to keep the has_side_effect=False for assume as that's the right thing to do, but removed assume from debug_assert as it's always DCE'd.

@modularbot
Copy link
Collaborator

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

@modularbot modularbot added merged-internally Indicates that this pull request has been merged internally merged-externally Merged externally in public mojo repo labels Jan 9, 2025
@modularbot
Copy link
Collaborator

Landed in 7a05c5c! Thank you for your contribution 🎉

modularbot pushed a commit that referenced this pull request Jan 11, 2025
[External] [stdlib] Mark assume as has_side_effect=False

To enable DCE when calling `assume`

Co-authored-by: soraros <[email protected]>
Closes #3658
MODULAR_ORIG_COMMIT_REV_ID: 6e6a12d09ecfa9e75c3c0bb75b383df01a205e62
@modularbot modularbot closed this Jan 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked by another issue that must be resolved first imported-internally Signals that a given pull request has been imported internally. merged-externally Merged externally in public mojo repo merged-internally Indicates that this pull request has been merged internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants