Skip to content

replace @setCold with @cold #5177

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

Closed
andrewrk opened this issue Apr 26, 2020 · 19 comments
Closed

replace @setCold with @cold #5177

andrewrk opened this issue Apr 26, 2020 · 19 comments
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@andrewrk
Copy link
Member

Status quo:

@setCold(is_cold: bool) void

Tells the optimizer that a function is rarely called.

Proposal:

@cold() void

Annotates that it is relatively uncommon for control flow to reach this point. Similar to unreachable, but only communicates probability. It communicates a willingness to compromise performance of the cold path in order to improve performance of the hot path.

This makes #489 unnecessary. Instead of:

if (@expect(foo, true)) {
   bar();
} else {
   baz();
}

With this proposal:

if (foo) {
    bar();
} else {
    @cold();
    baz();
}
@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Apr 26, 2020
@andrewrk andrewrk added this to the 0.7.0 milestone Apr 26, 2020
@ghost ghost mentioned this issue May 1, 2020
@andrewrk andrewrk added the accepted This proposal is planned. label May 1, 2020
@SpexGuy
Copy link
Contributor

SpexGuy commented May 1, 2020

How does this work with comptime-known coldness? The old approach would have been:

fn foo() void {
    @setCold(comptime_foo_is_cold);
}

But with the new approach:

fn foo() void {
    if (comptime_foo_is_cold) {
        @cold(); // doesn't work, applies to the if body.
    }
    // function implementation
}

@SpexGuy
Copy link
Contributor

SpexGuy commented May 1, 2020

I guess this might work, since technically there's no scope:

fn foo() void {
    if (comptime_foo_is_cold) @cold();
    // function implementation
}

@andrewrk
Copy link
Member Author

andrewrk commented May 1, 2020

That later example would still apply to the then-body of the if statement, communicating that comptime_foo_is_cold is unlikely. Which if the value is comptime-known is meaningless and ignored since the compiler knows precisely whether the branch is taken.

This proposal removes the ability to specify comptime-known coldness. I would be happy to reconsider if there is a convincing enough use case for comptime-known coldness.

@fengb
Copy link
Contributor

fengb commented May 1, 2020

ZeeAlloc has manual size tuning using @setCold: fengb/zee_alloc@b11ee8e#diff-d2841187941f93280c97ecaafd02aeb4R292

@andrewrk andrewrk removed the accepted This proposal is planned. label May 1, 2020
@andrewrk
Copy link
Member Author

andrewrk commented May 1, 2020

Thanks for the example. OK I'll amend the proposal to have @cold accept a comptime argument, same as @setCold. So the proposal is now essentially:

  • rename @setCold to @cold
  • make it apply per-branch-scope rather than per-function-scope

@ghost
Copy link

ghost commented May 3, 2020

There's a solution over at #5239 -- simple configuration is concise (@{cold};), and doesn't look deceptively like a function call; also, comptime configuration is always possible, but not so easy that it encourages users to go insane with it.

@andrewrk andrewrk modified the milestones: 0.7.0, 0.8.0 Oct 27, 2020
@andrewrk
Copy link
Member Author

btw I noticed that LLVM has a way to mark arbitrary code paths as cold: https://llvm.org/docs/LangRef.html#assume-operand-bundles

@andrewrk andrewrk added the accepted This proposal is planned. label Mar 25, 2021
@andrewrk andrewrk modified the milestones: 0.8.0, 0.9.0 Mar 25, 2021
@andrewrk andrewrk modified the milestones: 0.9.0, 0.10.0 May 19, 2021
@lambdadog
Copy link

lambdadog commented Sep 10, 2022

As I understand it, @setCold was initially implemented to replace the coldcc calling convention (although it doesn't seem it actually uses the coldcc calling convention these days, looking at the IR and assembly output...) -- will @cold in the top level of a function body push said function to use coldcc calling convention, or are these to be considered separate concerns?

Ideally, I'd like to see callconv(.Cold) or something similar to C's preserve_most calling convention for when you consider it important to optimization to not clobber your registers, but I acknowledge that Zig is a general purpose systems programming language and my usecase (high performance interpreters) in which I'm heavily scrutinizing the output the compiler gives me is unusual.

@lambdadog
Copy link

Okay, so as per #6556 and #6596, callconv(.Cold) was removed as it was believed @setCold should use it internally. This was never implemented, as far as I can tell.

Overall I'm of the opinion there are two options:

  • Reintroduce callconv(.Cold), as @setCold will no longer exist to mark a function as cold, as it was intended to.
  • Apply coldcc calling convention when within a @cold branch.

I think these are both solid options and that it may be one of the rare cases where it might actually be worthwhile to do both -- treating callconv(.Cold) as similar to callconv(.Inline) when functions can also be inlined automatically by the compiler -- we want the compiler to generate the best possible code it can automatically, but also give the user the ability to optimize their hottest codepaths by hand if necessary.

@tecanec
Copy link
Contributor

tecanec commented Dec 24, 2022

I think I have another use case for @expect: Hot loops.

Let's say we're making a loop which we want to iterate, say, a billion times per second, and for at least a few thousand iterations per loop entry. Every single μop matters, and we want the optimizer to prioritize maximum iterations per second over anything else, including exiting the loop.

With @expect, this is as simple as putting it at the loop condition. However, for something like @cold, which applies for the whole block, this becomes more difficult to do elegantly. Best I can think of is a while (true) loop containing a cold if with a break, but I think that goes against the zen by not favoring the canonical way of doing the conditional exit of a while loop.

I suppose an alternative solution would be letting @cold accept an enum rather than a boolean. The enum could be something like enum { cold, neutral, hot }, allowing both hot and cold paths to be designated. As these describe the relative temperature between two code paths rather than their absolute temperatures, marking one path as hot would implicitly make the other cold, and vice versa.

@anyputer
Copy link

anyputer commented Jun 3, 2023

Consider having an unlikely or cold keyword instead of a @cold() builtin, that can be used like this:

if (foo) {
    std.debug.print("this is probably going to happen", .{});
} else unlikely {
    std.debug.print("this would fit so nicely into the language!", .{});
}
fn panic() noreturn {
    unlikely {
        // the indentation here feels a bit gross...
        std.debug.print("boo!", .{});    
    }
}
switch (foo) {
    42 => "life",
    69, 666 => unlikely "funny number",
    else => unlikely "anything else",
}

The pragmas proposal is pretty interesting too.

@dweiller
Copy link
Contributor

btw I noticed that LLVM has a way to mark arbitrary code paths as cold: https://llvm.org/docs/LangRef.html#assume-operand-bundles

I find the section on assume operator bundles unclear - one interpretation is that putting an operand bundle with the "cold" tag on an assume intrinsic lets the optimizer assume that the location containing the assume intrinsic has the "cold" tag, rather than making particular block cold. If that's the case, another option might be the expect intrinsic or the expect with probability intrinsic.

CrepeGoat added a commit to CrepeGoat/fast-maths that referenced this issue Aug 8, 2023
…c path"

This reverts commit f6678f0.

Specifically, `@setCold` only affects full function scopes.
The zig language feature for marking specific scopes as "cold" is being
tracked in ziglang/zig#5177.
@16hournaps
Copy link

16hournaps commented Mar 21, 2024

I was just now writing some @setCold zig code, and realized it is very confusing because my brain sees a word cold, but in fact @setCold(false) -> Hot path. So it takes a couple of seconds to comprehend that even though it says cold in reality it is in fact hot.

More clear syntax would be highly appreciated even though not necessary as it currently works ok 👍

@Inve1951
Copy link
Contributor

Isn't @setCold(false) currently a no-op?

@16hournaps
Copy link

Isn't @setCold(false) currently a no-op?

If that's the case then I'm even more confused by the syntax. Thanks for pointing that one out.

@lambdadog
Copy link

Yes, this is for the case that you want to set coldness via a comptime-known value as noted above in the thread.

@cryptocode
Copy link
Contributor

Should there be an equivalent of __builtin_expect_with_probability? It can be used to inform backends with branch weight metadata and it appears to be used to guide outlining (function extraction): https://llvm.org/devmtg/2022-11/slides/QuickTalk12-ExpectingTheExpected.pdf

@Rexicon226
Copy link
Contributor

Should there be an equivalent of __builtin_expect_with_probability? It can be used to inform backends with branch weight metadata and it appears to be used to guide outlining (function extraction): https://llvm.org/devmtg/2022-11/slides/QuickTalk12-ExpectingTheExpected.pdf

See #489

@andrewrk andrewrk removed the accepted This proposal is planned. label Jul 15, 2024
@andrewrk andrewrk modified the milestones: 0.16.0, 0.14.0 Jul 15, 2024
@andrewrk andrewrk changed the title replace @setCold() with @cold() replace @setCold with @cold Jul 15, 2024
@andrewrk
Copy link
Member Author

Rejected in favor of #21148.

@andrewrk andrewrk closed this as not planned Won't fix, can't repro, duplicate, stale Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

No branches or pull requests