Repository navigation
Improve handling of immediate mode arguments #248
Description
Activity
@alexcrichton I have lifted this issue to the rust-lang/rust repo.
I think that handling immediate mode arguments properly is something we should attempt to fix before stabilization somehow.
That is, either we get
foo(const imm8: T), or we should enumerate the intrinsics with immediate mode arguments and special-case them in trans somehow.Thanks! I'm not sure I 100% agree we should block stabilization, but we can always sort out these details later :)
I guess my thoughts are more aligned with either:
- don't stabilizing these intrinsics at first (some of them are very useful, so I'd rather not do this)
- use the compiler to enforce that the arguments passed to these are compile-time constants somehow (not necessarily a language feature, but either an unstable attribute or just implementing these as compiler-intrinsics and exposing them via
stdsimd).
Both choices probably will mean that the strongly-typed SIMD wrapper over stdsimd won't be able to offer the same guarantees that stdsimd does (e.g. the wrapper will need to use the
constify!macros internally because otherwise it can't map run-time arguments to compile-time constants).The idea of doing this in the compiler is what @eddyb basically mentioned in #190 .
Oh right yeah I should mention that if we had a mechanism I'd prefer to stabilize these with "requires a constant semantics" but I'm under the impression that such a mechanism would be pretty far off, and in the meantime our local workaround isn't too bad.
Oh I thought that we had a mechanism for this in the compiler because the prefetch intrinsic (and others) require this, but it looks like the
core::intrinsicsmodule works around this by adding an intrinsic for every combination of constant arguments.... :D- prefetch_read_data
- prefetch_read_instruction
- prefetch_write_data
- prefetch_write_instruction
(on clang its just
__builtin_prefect(....)and clang tests that the arguments are constants or emits a "nice" monomorphization time error)I don't think adding
core::intrinsics::bextri_imm8_0...core::intrinsics::bextri_imm8_16384is an option :D@eddyb ?
We do have the mechanism but it's specifically only used for shuffles right now.
Checking/promotion-to-guaranteed-constant: https://github.com/rust-lang/rust/blob/07ea2604075d6f896addce0e6949c7cf25dd3715/src/librustc_mir/transform/qualify_consts.rs#L760-L768 (oh come on GitHub... no pretty code?)
Custom lowering of MIR call arguments (this is only needed because arrays are not immediates - for most of the usecases you care about, special-casing this is unnecessary): https://github.com/rust-lang/rust/blob/07ea2604075d6f896addce0e6949c7cf25dd3715/src/librustc_trans/mir/block.rs#L521-L539
So it'd only be a few more lines to add more intrinsics to the checking/promotion side.
We could even put the information inlibrustc_platform_intrinsicsand read it from there.@eddyb do you think this could be implemented as a stdlib-internal function attribute (e.g. that specifies the argument name) or as a function argument attribute (these are not a thing, right?) ?
That way we could keep doing things in the library without having to mess with
librustc_platform_intrinsics(adding stuff here is not hard, but it raises the barrier of contributing tostdsimdquite a bit, and right now, we need all the help we can get)@gnzlbg The problem as I see it is that
librustc_platform_intrinsicsis wrong if it allows you to call one of the intrinsics without a constant value. But sure, we could have an attribute on the intrinsic itself, holding argument indices or something.
Attributes prefixed withrustc_are perma-unstable and we can easily use one of those.@eddyb do you know how easy it would be to experiment with an attribute like this? (aka how hard it would be to implement it)
I'm sort of also worried about things like:
let f: fn(..) = stdsimd::vendor::function_that_requires_constant;
In a situation like that, we presumably want to error, right? Are there other locations in the compiler that will be affected by such "this argument must be a constant" logic?
@alexcrichton Depends if you want to apply on just the intrinsic or also on some wrapper function and have that wrapper function monomorphized. The latter is harder and basically const generics.
@eddyb in our case it's all wrapper functions (which transitively call the intrinsic). But ok thanks for the info!
Oh, sorry, I was getting confused between the intrinsics and the wrappers. There isn't much to do there before
constgenerics lands and even then thisconst arg: Tfeature would need an RFC.I've opened up rust-lang/rust#48018 with @eddyb's strategy from above which adds a new unstable attribute which can guarantee that arguments to a call are constant. That I think is the "biggest hole" today but there's still two pieces remaining:
- The attribute doesn't disallow acquiring function pointers, so those can still be taken and passed.
- We can't actually take advantage of it yet in the definition of the various intrinsics. We'll need more compiler support eventually for removing the
constifymacros, and for now we'll have to leave them all as-is.
Aside from function pointers, there's also the
Fn*traits that shouldn't be implemented.In the mean time this might be a solution that we could apply after stabilization: https://internals.rust-lang.org/t/pre-rfc-const-function-arguments/6709/22
18 remaining items
We could reject anything but integer literals and named constants -- but I assume that would be a massive breaking change.
I don't think anything else is used much or even at all.
Okay, let's see what crater says: rust-lang/rust#80759
since we already have stabilized intrinsics with such arguments
Which I am very unhappy about, but I guess that ship has sailed. I still think we should deprecate them and move to const generics, which exist exactly for the use case of having a function that is generic over a constant value, but that's just my personal preference.
The fact that these intrinsics were stabilised seems like an abuse of the RFC process, as they require a new language feature that was never properly discussed in public, nor explicitly approved. I strongly agree that const arguments should be deprecated, as they're a hack that was a stopgap before const generics were ready for use. Whether one prefers the syntax or not is irrelevant: new language features should be approved via the RFC process.
It seems like it would be very easy to define macros that act like const argument functions, modulo the
!, implemented using const generics. What motivation is there to support this in the language natively?EDIT: Just to be clear: I am annoyed at myself (and not anyone else) for not realizing that there was a plan to stabilize stuff in stdarch that uses rustc_args_required_const when all of that was discussed.
I do think something has gone wrong with the process. This should have been a much more visible change to people not keeping up with the intrinsics work.
Reacted by tesuji and Oli SchererI'm happy to block all new uses of
rustc_args_required_constand convert all unstable functions in stdarch that currently use it to use const generics instead.The question is, what should we do about the functions which are already stable? I can think of two possibilities:
- Leave them as they are. This is somewhat suboptimal since these functions contain massive
matchstatements that bloat the size of libcore. This has been shown to cause performance regressions in compilation time. - We could convert them to const generics, but mark them with an attribute to support calling them with the old function-like syntax just for backwards compatibility reasons. Essentially the compiler would magically rewrite
_mm_extract_ps(foo, CONST)to_mm_extract_ps<CONST>(foo). The old-style call syntax would be marked as deprecated and would be forbidden in 2021 edition code.
- Leave them as they are. This is somewhat suboptimal since these functions contain massive
The question is, what should we do about the functions which are already stable? I can think of two possibilities:
I agree the former leaves much to be desired. Perhaps something to the effect of the second option could be added to the 2021 edition planning document.
We could convert them to const generics, but mark them with an attribute to support calling them with the old function-like syntax just for backwards compatibility reasons.
Presumably
#[rustc_args_required_const]could just be changed to have this new behaviour.Following the discussion in rust-lang/rust#76001, one possible option (maybe just an intermediate solution) would be to turn
rustc_args_required_constarguments into inline consts. At least that would decouple them from the somewhat fragile mechanism of promotion, and instead use the much more well-defined mechanism of inline consts. (It would also allow us to clean up promotion, reducing technical debt.) This should be backwards-compatible, so it could also be done on old editions.My concern with inline consts is that they don't support referring to generic parameters of the containing
fn/impl. As far as I know that is only real (user-visible) difference between inline consts and explicit promotion.The size of
libcore_arch-*.rlib, build withcargo build --releaseonx86_64-unknown-linux-gnu, shrunk from 58MB just before initial conversion to const generics, to 40MB as of 35d17a6.The largest remaining wrappers are for avx512f intrinsics. Using MIR dump as a proxy for size in rlib, their size often exceeds 1MB, while most have merely a few kB.
I'll focus on avx512f.rs today, some of the constify macros it uses are huge.
We should probably make an MCP to the lang team so that a concious decision can be made about "some day in the future we will support something like the "const fn arguments" pre-RFC (and until the lang team has made a decision on that, let's have a moratorium on stabilizing any more of these functions, and I promise I'll stop talking about deprecating them until (if ever, hopefully not) a decision is made to forbid them).
Has a decision been made by the lang team? I believe that stabilization of "template"-based intrinsics should be blocked on this decision, otherwise we may end up with two incompatible ways of doing the same thing.
Also I want to note that AFAIK Rust has explicitly choose to follow vendor intrinsic APIs as close as possible, even though in some cases it makes sense to deviate from them (e.g. by using unsigned types instead of signed ones). Using "template" arguments for immediate mode arguments goes against this choice and I believe will only result in confusion down the line.
Have decision been made by the lang team? I believe that stabilization of "template"-based intrinsics should be blocked on this decision, otherwise we may end up with two incompatible ways of doing the same thing.
I don't think so, but it's also not really a problem since we are already using the non-generic version. Using a generic version is entirely in the space of the libs team as far as I can tell. So when a PR to bump stdarch in the rustc repo is opened, we can require a libs team FCP on it.
This is done.
Many intrinsics take immediate more argument (that is, some arguments must be compile-time constants).
We currently implement them by taking these arguments as run-time values, and "converting" them to compile-time values using a match statement with a match arm for each particular case:
If a user pass them a compile-time value the optimizer can select the appropriate match arm and remove the rest of the code.
Passing them a run-time value should generate an error, but with this approach the compiler silently accepts this code, and generates code for all match arms, potentially making binary sizes / compile-times explode.
Even when the user passes a compile-time constant to the immediate arguments, some intrinsics (AMD loves these) require match statements with >= 65k arms that destroy compile-times in all cases (from 2x to 3x increase in compile-times of the whole
stdsimdtest suite if one of this functions is used somewhere). The following intrinsics have such extremely large match arms:sse4a::_mm_extracti_si64sse4a::_mm_inserti_si64tbm::bextriThis issue blocks completing SSE4a in #40 and fixing #26 . It was previously discussed in #190 but never got an issue of its own.
Amongst the proposed solutions were to either implement these in the compilers or wait till const generics + extensions arrive (e.g.
foo(x: const i8)to requirexto be a compile-time constant).