add const_eval_select macro to reduce redundancy

[RalfJung]

I played around a bit with a macro to make const_eval_select invocations look a bit nicer and avoid repeating the argument lists. Here's what I got. What do you think?

I didn't apply this everywhere yet because I wanted to gather feedback first.

The second commit moves the macros from #132542 into a more sensible place. It didn't seem worth its own PR and would conflict with this PR if done separately.

Cc @oli-obk @saethlin @tgross35

try-job: dist-aarch64-msvc

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[RalfJung]

In many cases, const_eval_select is actually the entire body of the surrounding function. The current macro generates an expression as that seemed most natural, but an alternative worth considering is having a macro that basically generates a function with two bodies -- a const-body and a runtime-body.

Conceptually I prefer the expression-based version, but the function version would be easier to make "look good" in the macro.

[oli-obk]

The function based version does seem less redundant, but is a bit annoying when there is a shared section of the function, as you'd then have to use a nested function again.

Maybe closure syntax can be reused here for the args and return type?

[RalfJung]

Sure that is easy to do... it's an immediately invoked closure though, so it'll still be syntactically strange.

[RalfJung]

It may make more sense to do it a bit like the Miri callback macro, with something like

                    @capture {
                        arg1: i32 = arg1_val,
                        arg2: T = arg2_val,
                    } -> ReturnType:
                    if const {
                        // ...
                    } else {
                       // ...
                    }  

[oli-obk]

Hmm right. Would be neat if we could use closures in general (before this PR) and just have them not capture anything like when converting them to function pointers. Then we wouldn't need to specify types at all.

So this is an improvement over the status, so let's go with it and separately think about other things

[RalfJung]

Does const_eval_select support captureless closures?

So this is an improvement over the status, so let's go with it and separately think about other things

With the @captures syntax or what the PR does?

[oli-obk]

Does const_eval_select support captureless closures?

I do not believe so

With the @captures syntax or what the PR does?

Without, I think that style only makes sense if there are multiple such things like in miri callbacks

[RalfJung]

Without, I think that style only makes sense if there are multiple such things like in miri callbacks

There are multiple things though, namely the two arms of the if?

I think I like the capture more than the previous syntax since that looks like a function declaration which is confusing -- this is about defining which part of the surrounding environment can be used inside the if const, which IMO is better captured by @capture. Please take a look.

[oli-obk]

The return type on capture logic seems odd... maybe have a separate @return line?

Since the #[inline] will be repeated at every site, is there a use for specifying it?

The const/runtime code blocks could also be specified as @const and @runtime respectively, not sure what I like more

[RalfJung]

maybe have a separate @return line?

Or maybe if const -> T { ... } else { ... }?

Since the #[inline] will be repeated at every site, is there a use for specifying it?

I originally was going for perfectly matching the existing code, including the exact attributes.

But I can also add #[inline] automatically and only require annotations for further attributes.

The const/runtime code blocks could also be specified as @const and @runtime respectively, not sure what I like more

I considered that, but I like if const too much to give up on it so easily. ;)

[RalfJung]

I did the inline thing. I couldn't find a notation for the return type that I liked more than the current one. if const -> T doesn't work due to follow set restrictions, it'd have to be if const ->T:... which I guess is fine? But it's not really less weird than @capture {} -> T:.

[RalfJung]

@bors try

[tgross35]

@saethlin was suggesting the compiler could just automatically #[inline] anything with f128 or f16 as arguments. I'm not exactly sure how that would have to happen but it sounds nice...

[workingjubilee]

On ARM64EC, a lot of stuff has to generate thunks, @dpaoliello explained it here: #131781 (comment)

[RalfJung]

Yeah, but it's a pretty bad bug in that target if that means we have to add #[inline] everywhere a new LLVM type is used. These kind of externalities are not generally acceptable.

was suggesting the compiler could just automatically #[inline] anything with f128 or f16 as arguments. I'm not exactly sure how that would have to happen but it sounds nice...

We should only do that for this broken target though, since it could also have quite negative effects.

[tgross35]

was suggesting the compiler could just automatically #[inline] anything with f128 or f16 as arguments. I'm not exactly sure how that would have to happen but it sounds nice...

We should only do that for this broken target though, since it could also have quite negative effects.

Cranelift also needs it, and there are a handful of other targets that LLVM crashes on https://github.com/rust-lang/compiler-builtins/blob/e34429548a0d6a186264dd74fbfc24bd3dbef667/configure.rs#L63-L76. But yeah, it's not ideal.

[RalfJung]

Okay so support for these types generally is still pretty poor in LLVM, not just for that one target.

In other words, it's f16/f128 causing externalities here. In that case I agree, we should add some hacks to the compiler to make this more smooth for unrelated rustc development.

[RalfJung]

But also, why does this function even get codegen'd? It is const-only!

Maybe we should just always make the const arm of const_eval_select #[inline] to prevent it being fed to LLVM. (We already do that with the runtime arm to avoid the overheads of the extra fn call.)

[RalfJung]

@bors try

[bors]

⌛ Trying commit 613f53e with merge fecb30e...

[RalfJung]

This one makes me particularly happy. :) This is much nicer with the macro.

[bors]

☀️ Try build successful - checks-actions
Build commit: fecb30e (fecb30e858fa70192efbdb7f7d464956222749d4)

[RalfJung]

All right, looks like this helped. :)

@bors r=oli-obk

[bors]

📌 Commit 613f53e has been approved by oli-obk

It is now in the queue for this repository.

[saethlin]

@saethlin was suggesting the compiler could just automatically #[inline] anything with f128 or f16 as arguments. I'm not exactly sure how that would have to happen but it sounds nice...

I don't remember suggesting this, but all you'd need to do is add a check below this point:

rust/compiler/rustc_mir_transform/src/cross_crate_inline.rs

Line 32 in 096277e

// From this point on, it is valid to return true or false.

based on checking if any argument types are tcx.types.f16 or tcx.types.f128.