Skip to content

implement @expect builtin #19658

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

Merged
merged 8 commits into from
May 22, 2024
Merged

implement @expect builtin #19658

merged 8 commits into from
May 22, 2024

Conversation

Rexicon226
Copy link
Contributor

@Rexicon226 Rexicon226 commented Apr 15, 2024

closes #489

Why @expect takes a bool:

  • @expect affects branching codegen and you can only branch on a bool.
  • Expecting a specific value can be done with unreachable with respect to the optimizer.

Why @expect doesn't have a probability:

  • Probabilities don't make a practical difference in codegen vs. binary.
  • The switch usecase can't reliably predict codegen (could be a jump table, some could be comparisons, or it could be a mixture of both)

@daurnimator
Copy link
Contributor

Probabilities don't make a practical difference in codegen vs. binary.

Are you saying that LLVM's expect.with.probability doesn't do anything?

@kprotty
Copy link
Member

kprotty commented Apr 15, 2024

Expect with probability seems to affect switch cases but it's unclear how it does so practically compared to if.

@Techatrix
Copy link
Contributor

When there is no expected value nor probability, wouldn't a name like @likely instead of @expect be more fitting so that this builtin doesn't get mixed up to be an assertion or function like std.testing.expect?

@notcancername
Copy link
Contributor

notcancername commented Apr 16, 2024

For purposes of consistency with C (compiler __builtin_expect vs the common #define LIKELY(x) __builtin_expect(x, 1)), it seems better to name it likely.

@Rexicon226
Copy link
Contributor Author

I don't want likely as it implies an unlikely. Note that the only real purpose of this builtin is the unlikely version, and unlikely is a bit ugly imo.

I will change it up to accept a second field for true or false instead and keep expect as per @kprotty suggestion when we talked about it.

@notcancername
Copy link
Contributor

Since the user can implement a likely or unlikely pretty easily themselves when a second field is added:

inline fn likely(x: bool) bool {
    return @expect(x, true);
}
inline fn unlikely(x: bool) bool {
    return @expect(x, false);
}

I think this would be the best solution. Thanks! :)

@notcancername
Copy link
Contributor

notcancername commented Apr 16, 2024

Should likely and unlikely functions be part of the standard library? On one hand, they're one-liners any user can easily write, on the other hand, everyone writing their likely and unlikely functions is worse, a likely/unlikely micropackage would also suck. Discuss.

@Rexicon226
Copy link
Contributor Author

Thought about that yesterday, but I think making @expect take a second "expected" bool supports both likely and unlikely without a wrapper.

Copy link
Contributor

@clickingbuttons clickingbuttons left a comment

Choose a reason for hiding this comment

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

What about a nice sugary @expect(operand: anytype, expected: anytype) and add a check for @typeOf(operand) == @typeOf(expected)?

@Rexicon226
Copy link
Contributor Author

What about a nice sugary @expect(operand: anytype, expected: anytype) and add a check for @typeof(operand) == @typeof(expected)?

No, this will defeat the purpose of this builtin.

@Snektron
Copy link
Collaborator

Snektron commented May 7, 2024

image
nobody @expects the Spanish inquisition!!

@kprotty kprotty merged commit a7de02e into ziglang:master May 22, 2024
10 checks passed
@Rexicon226 Rexicon226 deleted the expect branch May 22, 2024 15:51
andrewrk added a commit that referenced this pull request May 22, 2024
This reverts commit a7de02e.

This did not implement the accepted proposal, and I did not sign off
on the changes. I would like a chance to review this, please.
@andrewrk
Copy link
Member

Reverted in 9be8a90. I would like a chance to review this please.

@Rexicon226 Rexicon226 restored the expect branch May 22, 2024 17:00
ryoppippi pushed a commit to ryoppippi/zig that referenced this pull request Jul 5, 2024
* implement `@expect`

* add docs

* add a second arg for expected bool

* fix typo

* move `expect` to use BinOp

* update to newer langref format
ryoppippi pushed a commit to ryoppippi/zig that referenced this pull request Jul 5, 2024
This reverts commit a7de02e.

This did not implement the accepted proposal, and I did not sign off
on the changes. I would like a chance to review this, please.
SammyJames pushed a commit to SammyJames/zig that referenced this pull request Aug 7, 2024
* implement `@expect`

* add docs

* add a second arg for expected bool

* fix typo

* move `expect` to use BinOp

* update to newer langref format
SammyJames pushed a commit to SammyJames/zig that referenced this pull request Aug 7, 2024
This reverts commit a7de02e.

This did not implement the accepted proposal, and I did not sign off
on the changes. I would like a chance to review this, please.
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.

@expect() hint to optimizer
8 participants