Add rustc_intrinsic_const_vector_arg attribute to allow vectors to be passed as constants #118980

GeorgeWort · 2023-12-15T14:04:32Z

This allows constant vectors using a repr(simd) type to be propagated
through to the backend by reusing the functionality used to do a similar
thing for the simd_shuffle intrinsic.

fix #118209

rustbot · 2023-12-15T14:04:40Z

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-12-15T14:04:42Z

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

workingjubilee

cc @bjorn3

tests/ui/internal-lints/rustc_intrinsic_const_vector_arg_calls.rs

bors · 2023-12-18T12:53:52Z

☔ The latest upstream changes (presumably #119070) made this pull request unmergeable. Please resolve the merge conflicts.

GeorgeWort · 2023-12-19T17:39:14Z

I have narrowed the scope of this attribute to only being applicable to simd types defined in the local crate as I do not know how to check if the type is a simd type when it is not local in HIR.

bors · 2023-12-26T05:19:04Z

☔ The latest upstream changes (presumably #119146) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2024-02-16T12:09:51Z

☔ The latest upstream changes (presumably #120500) made this pull request unmergeable. Please resolve the merge conflicts.

Dylan-DPC · 2024-02-19T13:40:46Z

@GeorgeWort can you address the CI test failure? once that's done you can comment with @rustbot ready to mark this as ready for review and we can push this forward

RalfJung · 2024-02-21T08:12:11Z

compiler/rustc_borrowck/src/type_check/mod.rs

This diff is much bigger than needed. See #121225 for a PR that makes some other intrinsics require a constant argument; this should share that same check.

In fact the even better alternative is probably to remove the simd_shuffle case entirely, and just add the new attribute to simd_shuffle.

RalfJung · 2024-02-21T08:13:59Z

compiler/rustc_codegen_ssa/src/mir/block.rs

        let mut copied_constant_arguments = vec![];
        'make_args: for (i, arg) in first_args.iter().enumerate() {
-            let mut op = self.codegen_operand(bx, &arg.node);
+            let mut op = if const_vec_arg_indexes.contains(&i) {
+                // Force the specified argument to be constant by using const-qualification to promote any complex rvalues to constant.


This comment is misleading. You are not promoting anything here, and you shouldn't. You are not forcing anything to be a constant either, you are relying on the fact that typecheck already already ensured that this is a constant.

RalfJung · 2024-02-21T08:15:12Z

tests/ui/internal-lints/rustc_intrinsic_const_vector_arg.rs

+    fn foo2(a: i8x2, b: i8);
+}
+
+#[rustc_intrinsic_const_vector_arg(0)] //~ ERROR  attribute should be applied to functions in `extern "unadjusted"` modules


Is there a reason this argument needs to be about const vectors?

If this were to become a general rustc_intrinsic_const_arg, then it could also be used for simd_insert/extract.

Jamesbarford · 2024-07-24T12:04:45Z

@rustbot ready

oli-obk · 2024-07-25T08:40:21Z

fwiw I don't think this should be done. We're just digging ourselves a bigger hole. This explicitly works around a type system limitation in ways that we may never be able to recover from. I'm aware of the precedent, but that doesn't really make it better to use it more widely.

If this stays assigned to me I will just close it and the corresponding issue. I am aware of the ugliness and limitations of using const generics right now, but these limitations are there for a reason, and further going around our stability rules without a long term plan and lang team approval seems just wrong.

Jamesbarford · 2024-07-25T11:20:12Z

fwiw I don't think this should be done. We're just digging ourselves a bigger hole. This explicitly works around a type system limitation in ways that we may never be able to recover from. I'm aware of the precedent, but that doesn't really make it better to use it more widely.

If this stays assigned to me I will just close it and the corresponding issue. I am aware of the ugliness and limitations of using const generics right now, but these limitations are there for a reason, and further going around our stability rules without a long term plan and lang team approval seems just wrong.

Thanks for your feedback. I appreciate your thoughts about this seeming like a work around the limitations of the type system. I could be mistaken on the following; this problem seems more with 'const vectors' as opposed to a problem const generics more broadly? Though I appreciate I could be wrong on that and that these are these in fact entwined? Also what stability features are being circumvented?

oli-obk · 2024-07-25T14:55:57Z

Since it allows complex computations to be done for the const argument (e.g. via const {}) blocks, these computations can fail and thus fail compilation. Normally that's fine, but when the computation depends on a generic parameter, it means that users will only see an error when they pick a specific concrete type (or constant) for that generic parameter. This can be arbitrarily deep in a function call chain, so it could be that the error happens in a random crate that you have no control over, but the situation that causes an error is not part of the signature. These are called monomorphization-time-errors and are quality wise not great and generally go against Rust's concept of specifying everything in the signature instead of doing duck-typing.

oli-obk · 2024-07-25T15:02:09Z

That said, if someone else wants to deal with this, that's fine. I just won't be able to do any compiler work that would unblock anything here in the next 4 months, and I don't just want to approve something just because it works and I don't have time to dig into it.

Also I would prefer having a long term strategy of T-libs, T-lang and SIMD folk for what is going to happen here, but I can totally see that there's no capacity for doing that work rn and we already have a hack so why not use it more.

RalfJung · 2024-07-25T16:07:21Z

Overall the situation is similar to simd_shuffle -- there we also punted on this and trust stdarch to do all the required checking. Unfortunately it turns out hardware is ~~even more terrible than we knew and can't just have a nice simple orthogonal instruction set because we can't have nice things~~ complicated so apparently ARM has these instructions like llvm.arm.neon.vqshiftsu.v8i8 that take two vectors where the second must be compile-time known, just like for simd_shuffle... and somehow then we only expose a version of this operation where the vector is a single constant N repeated 8 times? None of this makes sense to me, TBH.^^

But anyway, with simd_shuffle we were okay trusting stdarch to do all the required checks, so we could consider doing the same here. We'd have to make it clear that this is really just a hack to support the ~~terrible~~ APIs imposed on us by hardware vendors, and we expect stdarch to guarantee that these constants cannot fail to evaluate.
Given that generic_const_expr still seems way off, the simd_shuffle hack is the only realistic option we can offer here. (Or else tell stdarch that they just can't call llvm.arm.neon.vqshiftsu and similar intrinsics I guess, but that seems like a sad outcome.)
@rust-lang/types I'd be interested in your input here.

Regarding the PR itself, I am concerned that this seems to largely duplicate the simd_shuffle infrastructure instead of replacing it. IMO if we do this, we do it right: rip out all special-casing of simd_shuffle and make it use the attribute instead. And in fact this should cover simd_insert and simd_extract as well, which are part of the same hack. const_vector_arg is a misnomer, this should be const_arg; it is up to the intrinsic whether it wants a vector there or some other compile-time constant.

I wouldn't mind having other people involved in the discussion though, let's get a compiler reviewer.
r? compiler

RalfJung · 2024-07-25T16:11:14Z

compiler/rustc_codegen_ssa/src/mir/constant.rs

-    /// process constant containing SIMD shuffle indices
-    pub fn simd_shuffle_indices(
+    /// process constant SIMD vector or constant containing SIMD shuffle indices
+    pub fn early_evaluate_const_vector(


I find "early" here quite confusing, I don't think we otherwise use this adjective to refer to compile-time evaluation. Why not just evaluate_const_vector?

RalfJung · 2024-07-25T16:16:01Z

compiler/rustc_codegen_ssa/src/mir/constant.rs

+                // call([1, 0]);
+                //
+                // And outputting: @call(<2 x i32> <i32 0, i32 1>)
+                // thus treating them the same if they are a const vector


I don't understand any part of this comment. What's call? Why does it not involve defn at all? Also, the code below needs comments explaining how it relates to these two cases.

But really, the biggest issue is that a nested loop makes no sense. There should be only one loop, the one that goes over all vector elements. The only difference between the two representations is that in one case we need one extra unwrap_branch()[0] since the "struct wrapping an array" adds one extra layer to the tree.

RalfJung · 2024-07-25T16:17:31Z

compiler/rustc_codegen_ssa/src/mir/block.rs

+                if let mir::Operand::Constant(constant) = &arg.node
+                    && constant.ty().is_simd()
+                {
+                    let (llval, ty) = self.early_evaluate_const_vector(bx, &constant);


So is there no LLVM intrinsic that needs a constant integer? It's only ever vectors? That seems odd.

RalfJung · 2024-07-25T16:19:01Z

tests/ui/internal-lints/rustc_intrinsic_const_vector_arg_calls.rs

This test has nothing to do with any lint, does it? It checks that the attribute actually works?

oli-obk · 2024-07-25T16:22:36Z

guarantee that these constants cannot fail to evaluate.

It has an awful lot of static asserts, tho outside the type system

RalfJung · 2024-07-25T16:27:10Z

compiler/rustc_codegen_ssa/src/mir/block.rs

        let mut copied_constant_arguments = vec![];
        'make_args: for (i, arg) in first_args.iter().enumerate() {
-            let mut op = self.codegen_operand(bx, &arg.node);
+            let mut op = if const_vec_arg_indexes.contains(&i) {


I think you can just remove this if. It's always correct to use this translation if the value happens to be a constant SIMD vector, right?

In fact, should we maybe do this in OperandRef::from_const? Then it just happens for all constant SIMD vectors? That would in fact make the attribute entirely optional -- a nice-to-have to prevent LLVM errors, but not actually required to unblock stdarch.

RalfJung · 2024-07-25T16:37:05Z

It has an awful lot of static asserts, tho outside the type system

Yeah, tons of inline consts with assertions. That's even available on stable now so it should be acceptable.

After reading through the PR (against better judgment, but this is more fun than the things I should be doing ;), I think there's actually two largely orthogonal issues here:

Make it so that when any intrinsic is called with a const { ... } of SIMD type (with integer/float elements, not pointers), we generate an LLVM immediate, as that's what these kinds of intrinsics expect. We already do that for const { ... } of integer type, which I assume is why the question only comes up for SIMD vectors. This change can be done almost entirely inside codegen -- no new attribute needed. In fact I think we can just do this for all constants of appropriate SIMD type everywhere, why would we only do it for constants passed to intrinsics? It does need simd_shuffle_indices to be generalized to something like immediate_const_vector, but that's about it. This unblocks stdarch, they can then add const { ... } blocks for all calls to vqshiftsu and similar intrinsics and codegen will do the right thing. (That PR will definitely need a codegen test to check that we generate the right IR -- that's dearly missing from the current PR.)
That leaves us in a somewhat fragile situation where if one of these intrinsics is called with a non-const argument, we might trip some odd failures in LLVM. We could decide to do something about it with something like this attribute, but honestly -- for these super-internal LLVM intrinsics, I don't think that's worth it. For our own intrinsics, we know which arguments need to be const, so we can (and do) check that. But for LLVM's intrinsics, someone has to tell us, and that someone can only be stdarch, and if they forget adding a const { ... } somewhere then likely they would also forget adding the attribute. So honestly I don't see the point of the attribute. I suspect for intrinsics that require constants of integer types, this is already the status quo -- stdarch just wraps them in const { ... }, and codegen happens to always make that an LLVM immediate, which makes the LLVM backend happy, and there is no need for an attribute that double-checks that these arguments are really constants.

RalfJung · 2024-07-25T16:50:44Z

So @GeorgeWort I would propose that you make a new PR that does the first part of what I described in my previous issue. I'd suggest changing OperandRef::from_const so that a ConstValue::Indirect of SIMD type with integer/float elements is instead passed through simd_shuffle_indices (with the changes you already made in this PR to support both kinds of repr(simd)) and returned as OperandValue::Immediate. Then this special case can be removed, and rust-lang/stdarch#1503 without the new attributes should suffice to fix the issue.

bors · 2024-07-29T05:44:15Z

☔ The latest upstream changes (presumably #125443) made this pull request unmergeable. Please resolve the merge conflicts.

Jamesbarford · 2024-07-29T13:24:37Z

Thanks @RalfJung, hope the things you should be doing eventually get done! I have avoided replying to your individual comments on this PR as it seems we're looking at a different approach to tackle this problem.

What you have stated makes sense to have a generalised place, potentially named immediate_const_vector, to handle this logic without an additional attribute. Yes, a test certainly is needed.

With respect to odd failures with LLVM, in debug mode we are currently experiencing; LLVM ERROR: Cannot select: <instruction_name> as the const vector is not getting put into the correct LLVM const vector form. Thus your proposed changes should theoretically be a step in a better direction.

Are there any further places in the code that you could point me to where I should investigate further?

@oli-obk what are your thought's on these suggestions? Do they seem reasonable to you?

RalfJung · 2024-07-29T13:56:54Z

Are there any further places in the code that you could point me to where I should investigate further?

Not sure what else is there to investigate; as far as I am concerned, we can move to implementation. That's a fairly local change inside the codegen backend, without any new guarantees or language-level surface, doing for vector constants what we already happen to do for integer constants -- generating them as LLVM immediates.

I'm not an LLVM expert though so maybe there's something I missed. @nikic @Amanieu @workingjubilee does this sound like a reasonable plan?

nikic · 2024-07-29T14:04:07Z

@RalfJung Sounds reasonable to me.

Amanieu · 2024-07-29T14:14:38Z

complicated so apparently ARM has these instructions like llvm.arm.neon.vqshiftsu.v8i8 that take two vectors where the second must be compile-time known, just like for simd_shuffle... and somehow then we only expose a version of this operation where the vector is a single constant N repeated 8 times? None of this makes sense to me, TBH.^^

The CPU instruction only takes an immediate constant. For some reason LLVM decided to make their intrinsic take a vector and then require all elements of that vector to have the same value (which must also be a constant). While this is a rather bizarre choice, in the end all we have to do is ensure that the LLVM intrinsic gets its immediate constant vector from our const eval.

Jamesbarford · 2024-07-30T08:29:48Z

Thanks, I will close this PR and commence work on a new branch and open a new pull request against the same issue.

RalfJung · 2024-07-30T09:08:50Z

Please mention the new PR in #118209 once it exists. Backlinks won't trigger notifications. :)

rustbot assigned wesleywiser Dec 15, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 15, 2023