Skip to content
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

SIMD groundwork part 1 #27169

Merged
merged 40 commits into from
Aug 18, 2015
Merged

SIMD groundwork part 1 #27169

merged 40 commits into from
Aug 18, 2015

Conversation

huonw
Copy link
Member

@huonw huonw commented Jul 20, 2015

This implements rust-lang/rfcs#1199 (except for doing all the platform intrinsics).

Things remaining for SIMD (not necessarily in this PR):

  • I (@huonw) am signed up to ensure the compiler matches the RFC, when it lands
  • the platform specific intrinsics aren't properly type checked at the moment (LLVM will throw a "random" assertion)
  • there's a lot of useful intrinsics that are missing, including whole platforms (mips, powerpc)
  • the target-feature cfg detection/adding is not so great at the moment
  • I think the platform specific intrinsics should go in their own extern ABI (i.e. not "rust-intrinsic")

(I'm adjusting the RFC to reflect the latter.)

I think it would be very nice for this to land without requiring the RFC to land first, because of the first point, and because this is the only way for any further work to happen/be experimented with, without requiring people to build/install/multirust a compiler from a custom branch.

r? @alexcrichton

let llet = in_memory_type_of(cx, t.simd_type(cx.tcx()));
let elem_ty = match t.simd_machine_type(cx.tcx()) {
Some(e) => e,
None => cx.sess().fatal("monomorphising SIMD type to an incompatible element")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ugly... is allowing generic SIMD types actually important in practice? It seems like you could accomplish most of what is necessary with traits and maybe a few macros.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it isn't so great, but note that in practice type safety is ensured via traits, e.g. struct Simd4<T: SimdElem>(T, T, T, T);.

This is the major difference to our current #[simd] scheme. Being generic is extremely useful, or else there is some extreme code-duplication and the compiler/library can't "synthesize" random types for helpers easily (i.e. forcing that duplication), e.g. fn get_high<T: SimdElem>(Simd4<T>) -> Simd2<T> works with any T but would require manually writing out for every Tx4 concrete type. (It'd be good to keep this sort of "high level" discussion on rust-lang/rfcs#1199 .)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not sure this is a great... but okay, I'll move the discussion.

@huonw huonw force-pushed the simd branch 2 times, most recently from 68fb467 to 88cdfbd Compare July 20, 2015 22:56

fn features_contain(sess: &Session, s: &str) -> bool {
sess.target.target.options.features.contains(s) ||
sess.opts.cg.target_feature.contains(s)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't querying LLVM the "most robust" thing to do here? I guess all of these are disabled by default, though?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're all disabled by default, yeah; and querying LLVM doesn't seem overly possible, unfortunately. You have a better idea about LLVM's structure so maybe you can wrangle something, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you can get ahold of a TargetMachineRef, get ahold of the MCSubtargetInfo, get the list of features, and then test for various features being available.

It's probably not super critical that this happens immediately though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment to this function with the results of this investigation? Basically something along the lines of "we'd love to do X but we couldn't do it because of Y, hence we're doing the 'poor mans' version Z"

@alexcrichton
Copy link
Member

Nice work @huonw!

I think the platform specific intrinsics should go in their own extern ABI (i.e. not "rust-intrinsic")

I agree, not having the namespace of "simd_" made me a tad uneasy.

I think it would be very nice for this to land without requiring the RFC to land first, because of the first point, and because this is the only way for any further work to happen/be experimented with, without requiring people to build/install/multirust a compiler from a custom branch.

I would personally be ok with this, but I'm curious to hear what others think as well? Also, do you think the RFC basically has "broad consensus" modulo minor details by this point? If there are still some somewhat contentious decisions here and there it may be worth waiting a little longer before landing this, but if we're all in agreement pretty much then any future minor updates can be easily reflected here (as everything is unstable anyway).

cc @rust-lang/lang

@bors
Copy link
Contributor

bors commented Jul 22, 2015

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

@bors
Copy link
Contributor

bors commented Jul 31, 2015

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

@huonw huonw force-pushed the simd branch 3 times, most recently from fe76451 to 16c5c37 Compare August 6, 2015 22:29
@huonw
Copy link
Member Author

huonw commented Aug 6, 2015

Ok, rebased and updated to be better, including

  • intrinsics for casts and arithmetic (i.e. operators don't work automatically for SIMD types: they have to go via the normal traits)
  • a special platform-intrinsic extern ABI for these intrinsics,
  • type-checking the platform-specific intrinsics,
  • some general refactorings

(I'm updating the RFC to ensure it matches the things I've learned while implementing, as we speak.)

cesarb added a commit to cesarb/blake2-rfc that referenced this pull request Aug 7, 2015
@bors
Copy link
Contributor

bors commented Aug 7, 2015

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

cesarb added a commit to cesarb/blake2-rfc that referenced this pull request Aug 8, 2015
@@ -897,7 +906,41 @@ pub fn trans_intrinsic_call<'a, 'blk, 'tcx>(mut bcx: Block<'blk, 'tcx>,

}

(_, _) => ccx.sess().span_bug(foreign_item.span, "unknown intrinsic")
(_, _) => {
match Intrinsic::find(tcx, &name) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this de-indent a level via:

let intr = match ... {
    None => ccx.sess().span_bug(...),
    Some(intr) => intr,
};

@alexcrichton
Copy link
Member

Looking good to me! Just a minor nit so far

@huonw
Copy link
Member Author

huonw commented Aug 13, 2015

Updated (will rebase in a bit), includes addressing the nit, adding a pile of platform intrinsic definitions (gets much of x86-64 from SSE to AVX2 and ARM NEON), and adding tests.

@huonw
Copy link
Member Author

huonw commented Aug 13, 2015

(There's still some more tests I want to add, particularly run-pass tests making sure the various generic intrinsics behave sanely.)

@@ -223,7 +224,14 @@ pub fn sizing_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, t: Ty<'tcx>) -> Typ

ty::TyStruct(..) => {
if t.is_simd(cx.tcx()) {
let llet = type_of(cx, t.simd_type(cx.tcx()));
let e = t.simd_type(cx.tcx());
println!("sizing_type_of: simd: {}", e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a stray debugging println!, which is hit while compiling libcore.

huonw added 5 commits August 17, 2015 14:48
Overload the operators using the traits so that things mostly keep
working during the deprecation period.
I.e. the signature now must be

    fn simd_shuffleNNN<T, U>(x: T, y: T, idx: [u32; NNN]) -> U;

(modulo names.)
Factor out common pieces, follow `expected ..., found ...` convention
everywhere.
@huonw
Copy link
Member Author

huonw commented Aug 17, 2015

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Aug 17, 2015

📌 Commit 02e9734 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Aug 17, 2015

⌛ Testing commit 02e9734 with merge e35fd74...

bors added a commit that referenced this pull request Aug 17, 2015
This implements rust-lang/rfcs#1199 (except for doing all the platform intrinsics).

Things remaining for SIMD (not necessarily in this PR):

- [x] I (@huonw) am signed up to ensure the compiler matches the RFC, when it lands
- [x] the platform specific intrinsics aren't properly type checked at the moment (LLVM will throw a "random" assertion)
- [ ] there's a lot of useful intrinsics that are missing, including whole platforms (mips, powerpc)
- [ ] the target-feature `cfg` detection/adding is not so great at the moment
- [x] I think the platform specific intrinsics should go in their own `extern` ABI (i.e. not `"rust-intrinsic"`)

(I'm adjusting the RFC to reflect the latter.)

I think it would be very nice for this to land without requiring the RFC to land first, because of the first point, and because this is the only way for any further work to happen/be experimented with, without requiring people to build/install/multirust a compiler from a custom branch.

r? @alexcrichton
@bors bors merged commit 02e9734 into rust-lang:master Aug 18, 2015
bors-servo pushed a commit to servo/euclid that referenced this pull request Aug 28, 2015
Change simd to repr simd & bump version number

With [SIMD groundwork part 1](rust-lang/rust#27169)
the simd feature is now repr_simd.

Bump the version number since this relies on the rust PR. 

r? @metajack

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/euclid/101)
<!-- Reviewable:end -->
bors-servo pushed a commit to servo/euclid that referenced this pull request Aug 29, 2015
Change simd to repr simd & bump version number

With [SIMD groundwork part 1](rust-lang/rust#27169)
the simd feature is now repr_simd.

Bump the version number since this relies on the rust PR. 

r? @metajack

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/euclid/101)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants