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

[tbm] fix bextri #190

Closed
wants to merge 1 commit into from
Closed

[tbm] fix bextri #190

wants to merge 1 commit into from

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Nov 14, 2017

No description provided.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 14, 2017

@alexcrichton so i've splitted the bextri fix here.

We could change the API of functions taking immediate values to:

fn bextri_u32<const imm: u32>(x: u32);

but what I'd like to see is:

fn bextri_u32(x: u32, const imm: u32);

@withoutboats @eddyb thoughts ?

@eddyb
Copy link
Member

eddyb commented Nov 14, 2017

We can special-case some intrinsics to require a constant argument - we've successfully done this for shuffles, which means they get a constant checking error instead of a post-monomorphization one.

As for the feature of an argument being constant... I think I'd prefer imm: const u32 - in the vein of impl Trait, const T used in an input type position would be an anonymous const generic.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 14, 2017

As for the feature of an argument being constant... I think I'd prefer imm: const u32 - in the vein of impl Trait, const T used in an input type position would be an anonymous const generic.

That looks great.


I want to add that this PR does not introduce this problem. The macros.rs file edited in this PR already contains some constify macros with match statements up to 255 match arms that we are already using throughout the library to hack our way around this. This is not a new problem; this PR just makes it worse.

From stdsimd's point-of-view, there are two main issues.

The first one is compile-times. bextri's constify macro requires 4096 match arms; for ~4 uses of bextri throughout the library this results in 2-3x larger compile times for cargo test compared against not using bextri at all, making bextri unusable in practice.

The second problem is safety. Some of the intrinsics require const argument. Ideally, users would get a type error when calling an intrinsic that requires a const argument with a non-const one.

I think that for stdsimd an unstable attribute to mark const arguments would already be enough to help with compile-times, diagnostics, and remove the constify! macros.

Whether this deserves a language level solution or not I don't know. stdsimd is not a typical library. This particular problem arises from trying to expose the intrinsics without changing the compiler much by using link_llvm_intrinsics. If we were to do this in the compiler, as you said, we could fix this problems in a nicer way.


@alexcrichton I don't know what I should do about this PR. I don't want it to rotten but I don't want it to slow compile-times by a factor of 3 either. Iff we are going to fix this in trans we should probably fix the other intrinsics requiring immediate arguments as well, but this significantly raises the barrier of entry for people wanting to contribute to the library an intrinsic that requires immediate mode arguments. IIRC tbm::bextri should already be in librustc_platform_intrinsics
(I put it there a while ago), so we could expose that one here directly, and I could special case it in rustc like @eddyb said to make sure that the arguments are const.

@eddyb
Copy link
Member

eddyb commented Nov 14, 2017

Note that the special-casing would still have to be made in the compiler, the difference with using the imm: const u32 syntax is that users could pass the constant argument through several calls.
Such a feature would require and rely on const generics, so just const generics could be used instead when they arrive, with a minor loss of ergonomics but not of generic programming functionality.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 14, 2017

@eddyb

Note that the special-casing would still have to be made in the compiler

Currently the constify! macros work without any special casing in the compiler whatsoever. Is that just pure luck? Or why would we still need to have any of these intrinsics in the compiler at all? (beyond shuffles and other generic intrinsics).

@eddyb
Copy link
Member

eddyb commented Nov 14, 2017

@gnzlbg I meant if you wanted to write them as const imm: u32, although I could see it "just work" for intrinsics, by passing them as regular arguments.
FWIW I'd prefer if we didn't expose LLVM intrinsics directly because of the "post-monomorphization error" behavior we get from that, and specify const requirements in the compiler.

@alexcrichton
Copy link
Member

Ok thanks for the PR @gnzlbg! I think though that this is a case where I'd personally prefer at least to hold out for a language feature like const x: u32 or something like that. I suspect we're quite aways away from enabling such a language feature, but I think that's ok in terms of this may not be that hotly desired of an intrinsic?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Nov 19, 2017

@alexcrichton I agree. The only workaround for the current state of affairs that I can think of is to use associated consts:

// stdsimd:
pub trait bextri_64 {
  const IMM: u64;
  fn bextri_u64(x: u64) -> u64 {
    llvm_bextri(x, Self::IMM)
  }
}

// user
struct V2();
impl bextri_u64 for V {
  const IMM: u64 = 2;
}

but... this is a weird thing to do this.

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.

3 participants