Add const-eval support for SIMD types, insert, and extract #64738

gnzlbg · 2019-09-24T14:16:02Z

This adds initial support for constant-evaluation of Abi::Vector types.

gnzlbg · 2019-09-24T14:18:51Z

@oli-obk this does the bare minimum for getting platform-intrinsics working and adds two "simple" intrinsics. I want initial feedback on this because I don't know what I'm doing. I feel like there are many different ways of doing the same thing with the constant evaluator.

oli-obk · 2019-09-24T14:52:05Z

This is pretty cool. I think it would be best if the experimentation happened in the miri repo instead of in the const evaluator. With that you can actually run cargo miri test on crates excercising SIMD

src/librustc_mir/interpret/terminator.rs

gnzlbg · 2019-09-24T15:01:20Z

I think it would be best if the experimentation happened in the miri repo instead of in the const evaluator.

I tried that but miri did not support platform-intrinsics, so I needed to add support for that here, and then, I also needed to add a test here, so I ended up with this :/

I'm really confused for when some intrinsics should be implemented in the const-evaluator vs in the miri repository.

src/test/ui/consts/const-eval/simd/insert_extract-fail.stderr

src/test/ui/consts/const-eval/simd/insert_extract.stderr

gnzlbg · 2019-09-24T15:08:56Z

Also notice that these intrinsics are all unstable anyways so they can't be used from stable Rust.

oli-obk · 2019-09-24T15:12:17Z

I tried that but miri did not support platform-intrinsics, so I needed to add support for that here, and then, I also needed to add a test here, so I ended up with this :/

Sometimes miri changes need smaller miri-engine changes. In this case the removal of the ABI check

I'm really confused for when some intrinsics should be implemented in the const-evaluator vs in the miri repository.

anything experimental should go into miri, anything that will never get stabilized should go into miri. Anything that has an RFC and generally a sound plan for stabilization should go into the const evaluator.

We can trivially move things from miri to the const evaluator when we need to in the future.

Also notice that these intrinsics are all unstable anyways so they can't be used from stable Rust.

I'm not worried about that. I just think it would allow you to iterate much faster if you did this in miri, and you could test it much more easily since you can run miri on regular crates instead of having to cook up artificial examples

rust-highfive · 2019-09-24T15:21:49Z

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-09-24T15:02:54.9108811Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-09-24T15:02:54.9380464Z ##[command]git config gc.auto 0
2019-09-24T15:02:54.9447037Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-09-24T15:02:54.9496457Z ##[command]git config --get-all http.proxy
2019-09-24T15:02:54.9651994Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64738/merge:refs/remotes/pull/64738/merge
---
2019-09-24T15:09:45.2012823Z    Compiling serde_json v1.0.40
2019-09-24T15:09:47.0923147Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-09-24T15:09:58.4645205Z     Finished release [optimized] target(s) in 1m 34s
2019-09-24T15:09:58.4747795Z tidy check
2019-09-24T15:09:58.8923400Z tidy error: /checkout/src/librustc_mir/interpret/intrinsics.rs:251: line longer than 100 chars
2019-09-24T15:10:00.5879242Z some tidy checks failed
2019-09-24T15:10:00.5885809Z 
2019-09-24T15:10:00.5885809Z 
2019-09-24T15:10:00.5887716Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-09-24T15:10:00.5888359Z 
2019-09-24T15:10:00.5888385Z 
2019-09-24T15:10:00.5894812Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-09-24T15:10:00.5894923Z Build completed unsuccessfully in 0:01:37
2019-09-24T15:10:00.5894923Z Build completed unsuccessfully in 0:01:37
2019-09-24T15:10:00.5947984Z == clock drift check ==
2019-09-24T15:10:00.5963246Z   local time: Tue Sep 24 15:10:00 UTC 2019
2019-09-24T15:10:00.6700520Z   network time: Tue, 24 Sep 2019 15:10:00 GMT
2019-09-24T15:10:00.6700664Z == end clock drift check ==
2019-09-24T15:10:01.9889240Z ##[error]Bash exited with code '1'.
2019-09-24T15:10:01.9930366Z ##[section]Starting: Checkout
2019-09-24T15:10:01.9932461Z ==============================================================================
2019-09-24T15:10:01.9932545Z Task         : Get sources
2019-09-24T15:10:01.9932602Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

src/librustc_mir/interpret/intrinsics.rs

src/librustc_mir/interpret/operand.rs

gnzlbg · 2019-09-25T11:40:59Z

So I think I've made all changes. Tests pass on my side.

src/test/ui/consts/const-eval/simd/extract-fail2.stderr

@oli-obk

Add const-eval support for SIMD types, insert, and extract This adds initial support for constant-evaluation of Abi::Vector types. r? @oli-obk

@ghost

Rollup of 6 pull requests Successful merges: - #62975 (Almost fully deprecate hir::map::Map.hir_to_node_id) - #64386 (use `sign` variable in abs and wrapping_abs methods) - #64508 (or-patterns: Push `PatKind/PatternKind::Or` at top level to HIR & HAIR) - #64738 (Add const-eval support for SIMD types, insert, and extract) - #64759 (Refactor mbe a tiny bit) - #64764 (Master is now 1.40 ) Failed merges: r? @ghost

eddyb · 2019-09-26T13:49:26Z

src/librustc_mir/interpret/operand.rs

+        &self, op: OpTy<'tcx, M::PointerTag>
+    ) -> (u64, &rustc::ty::TyS<'tcx>) {
+        if let layout::Abi::Vector { .. } = op.layout.abi {
+            (op.layout.ty.simd_size(*self.tcx) as _, op.layout.ty.simd_type(*self.tcx))


Any reason why this is not using the element count from Abi::Vector?

gnzlbg · 2019-09-26T16:20:04Z

These two methods are removed in the SIMD array support PR. I have to change them there anyways, and that PR added a method that does this IIRC.

RalfJung · 2019-11-13T09:53:38Z

src/librustc_mir/interpret/intrinsics.rs

@@ -239,7 +239,52 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
            "transmute" => {
                self.copy_op_transmute(args[0], dest)?;
            }
+            "simd_insert" => {
+                let index = self.read_scalar(args[1])?.to_u32()? as u64;
+                let scalar = args[2];


This is quite confusing to call something of type OpTy a "scalar". Those should have type Scalar, or this should be called somewhere else, or there should at least be a comment.

RalfJung · 2019-11-13T09:56:10Z

src/librustc_mir/interpret/operand.rs

@@ -335,6 +335,17 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
        }
    }

+    /// Read vector length and element type
+    pub fn read_vector_ty(


This doesn't even look at the actual "operand", right? It is just a layout function?

Then it (a) should be mixed up with all the other read_* methods as it is very different, and (b) shouldn't even be called "read" as it doesn't read from memory.

I am not sure what the best place is for such a method, but it is not operand.rs. Sounds more like a TyLayout method to me?

RalfJung · 2019-11-13T09:57:17Z

src/librustc_mir/interpret/terminator.rs

-                if caller_abi != Abi::RustIntrinsic {
-                    throw_unsup!(FunctionAbiMismatch(caller_abi, Abi::RustIntrinsic))
-                }


Uh, why this? Instead of throwing out this check entirely, you could have just added platform intrinsics to the whitelist.

novacrazy · 2020-01-03T21:31:23Z

Was this reverted recently anywhere? Seems to consider them non-const in the latest nightly 0a58f58

oli-obk · 2020-01-03T21:46:28Z

We now need rustc_const_unstable on the intrinsics. Not sure if stdsimd added that, but it should. Maybe rustc should alsohave regression tests

novacrazy · 2020-01-03T21:56:09Z

Is that something I can add to these intrinsics now?

extern "platform-intrinsic" {
    crate fn simd_insert<T, U>(x: T, idx: u32, val: U) -> T;
    crate fn simd_extract<T, U>(x: T, idx: u32) -> U;
}

oli-obk · 2020-01-03T22:06:40Z

Yes, there are instructions in the rustc guide

novacrazy · 2020-01-03T22:10:28Z

The two paragraphs for rustc_const_unstable in the rustc guide do not tell me how to mark these intrinsics as const, and extern functions can't be marked as const anyway.

oli-obk · 2020-01-03T22:31:51Z

The last paragraph in https://rust-lang.github.io/rustc-guide/stability.html?highlight=rustc_const#rustc_const_unstable mentions this. Adding the attribute will make the intrinsic const

oli-obk · 2020-01-03T22:32:24Z

Can you update the guide so it explains it in a way that would have been clearer?

novacrazy · 2020-01-03T22:37:23Z

error[E0734]: stability attributes may not be used outside of the standard library

I'm not a rustc dev, this is within an application. I should have opened an issue but I figured it was a simple mistake or an intentional revert so I asked here first. It seems I will need to open an issue now.

oli-obk · 2020-01-03T22:43:32Z

Huh? You should not need to use intrinsics outside stdsimd. Can't you use the intrinsics from libcore instead of declaring them yourself?

oli-obk · 2020-01-03T22:46:27Z

Did this work on stable or nightly without feature gates? If so, we seem to have exposed a stability hole, and that definitely deserves an issue

novacrazy · 2020-01-03T22:46:39Z

I don't believe these are exposed anywhere in libcore, none of the "platform-intrinsics" are. Specifically, I have a fork of packed_simd specifically for const insert/extract operations, which is what broke.

oli-obk · 2020-01-03T23:04:42Z

Some things are in core::arch (see https://doc.rust-lang.org/core/arch/index.html), not sure if these also show up there (they aren't in the docs, but they may be missing)

oli-obk · 2020-01-03T23:11:58Z

If you just want your code to work again in a hacky way, you can see in https://github.com/rust-lang/rust/blob/9ae6cedb8d1e37469be1434642a3e403fce50a03/src/test/ui/consts/const-eval/simd/insert_extract.rs how this is done and what feature gates are needed. I don't know much about intrinsics, @gnzlbg is there any plan for exposing enough API surface so packed_simd can move away from using extern "platform-intrinsics"?

novacrazy · 2020-01-03T23:25:17Z

It does seem as though adding:

#![feature(staged_api)] // and many more, but this was the additional feature
#![stable(feature = "const_simd_intrinsics_raygon", since = "1.33.7")]
extern "platform-intrinsic" {
    #[rustc_const_stable(
        feature = "const_simd_intrinsics_raygon",
        since = "1.3.37"
    )]
    crate fn simd_extract<T, U>(x: T, idx: u32) -> U;
}

works for making it const, but now there are hundreds of X has missing stability attribute errors for my packed_simd fork, most likely from staged_api. So that hack is a no-go.

gnzlbg · 2020-01-04T09:11:48Z

@gnzlbg is there any plan for exposing enough API surface so packed_simd can move away from using extern "platform-intrinsics"?

The plan is to move packed_simd into libcore. Within libcore, it will continue to use intrinsics to implement this functionality.

oli-obk · 2020-01-04T09:49:21Z

OK. @novacrazy I'm sorry, for now we can't support your use case without you needing to add all the stability flags

gnzlbg · 2020-01-04T15:00:43Z

@novacrazy I’ll try to update packed simd with the new flags so that you can rebase your fork, but feel free to submit a PR doing that since I won’t have time till Monday or so.

novacrazy · 2020-01-04T21:37:37Z

You don't have to make it a priority. I was able to revert most of my const usage of these yesterday, and I'm hoping LLVM can optimize the rest for now.

Add const-eval support for SIMD types, insert, and extract

03ac54a

rust-highfive assigned oli-obk Sep 24, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 24, 2019

gnzlbg added 2 commits September 24, 2019 16:48

Add a fail test

75d8199

Move tests to SIMD subdirectory

f0bbd2b

oli-obk reviewed Sep 24, 2019

View reviewed changes

src/librustc_mir/interpret/terminator.rs Outdated Show resolved Hide resolved

Add some more tests

e6239bb

gnzlbg commented Sep 24, 2019

View reviewed changes

src/test/ui/consts/const-eval/simd/insert_extract-fail.stderr Outdated Show resolved Hide resolved

gnzlbg commented Sep 24, 2019

View reviewed changes

src/test/ui/consts/const-eval/simd/insert_extract.stderr Outdated Show resolved Hide resolved

gnzlbg added 3 commits September 25, 2019 10:28

Allow simd_insert and simd_extract in const_fn

3a6e96e

Remove unreachable code

97ce904

Tidy

02b3234