Skip to content

Feature/round #34

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

Merged
merged 9 commits into from
Oct 13, 2020
Merged

Feature/round #34

merged 9 commits into from
Oct 13, 2020

Conversation

calebzulawski
Copy link
Member

Mostly resolves #23. I didn't handle rounding to integers since llvm.lround doesn't support vectors.

@Lokathor
Copy link
Contributor

If we have to_int_unchecked, then we should offer to_int that does the checking, even if we can't offer that universally (or ever) as a single instruction.

@calebzulawski
Copy link
Member Author

I agree, can we make that a separate issue and handle that in a separate PR though? It depends on comparison operations we don't have yet

@calebzulawski
Copy link
Member Author

Hm, I didn't think my fix through, I forgot that it's probably rounding the integer out of range. How do I obtain the min/max integers that can be represented as the float of the same size?

@Lokathor
Copy link
Contributor

Might just need magical constants, there only two sizes involved here, 32-bit and 64-bit

@calebzulawski
Copy link
Member Author

This error is odd, and only occurs on riscv64gc:

Intrinsic has incorrect return type!
i64 (i64)* @llvm.ceil.v2f32
in function _ZN9core_simd5round5f32x247_$LT$impl$u20$core_simd..vectors_f32..f32x2$GT$4ceil17hb0d59b075a4324ebE
LLVM ERROR: Broken function found, compilation aborted!

@workingjubilee
Copy link
Member

workingjubilee commented Oct 12, 2020

I feel like I should ping @programmerjake on this one for some reason. Given the RISCV-specific error, at least.

@calebzulawski
Copy link
Member Author

That's a good idea. It only happens with vector types so I'm guessing it's a rustc SIMD FFI bug, I have a minimized example I can use to submit an issue if it is in fact a rustc bug.

@programmerjake
Copy link
Member

AFAICT it is a rustc bug where all the FFI type lowering machinery is invoked on intrinsics where it shouldn't be.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

So, SIMD FFI issues are definitely a rustc side problem.

Options seem to be:

  1. cfg out the relevant code for that arch and file a bug to fix it later. :/
  2. Block this on fixing SIMD FFI. I think there's a mostly-done abandoned PR somewhere... but this seems to be a non-starter?

Probably regardless:

  1. File the rustc bug if it's not a total duplicate?

@calebzulawski
Copy link
Member Author

Since the bug is only riscv64gc (I couldn't figure out how to test risc32gc, since cross doesn't seem to support it), should we just disable it in CI pending this bug? We could cfg-out but I believe this is going to effect nearly every addition going forward.

@workingjubilee
Copy link
Member

If this affects almost everything going forward for riscv64gc then, isn't dropping the test effectively tantamount to dropping our support for that architecture? We can put that on the table as an option, but I want to be sure we're all aware of what we're putting on the table.

@calebzulawski
Copy link
Member Author

Well, we could polyfill everything, but I have a feeling that may actually be more work than fixing the rustc bug. I'm not proposing dropping support indefinitely, but temporarily disabling it in CI until we can reenable it or otherwise decide to polyfill.

@calebzulawski
Copy link
Member Author

And of course open a matching issue in stdsimd that makes it clear that issue is outstanding.

@workingjubilee
Copy link
Member

Alright. This seems inevitable but it's not like it's tier 1 and this is fixable.

@calebzulawski
Copy link
Member Author

Agreed, definitely inevitable. I'm hoping the fix in rustc isn't a big deal. Also, opened a corresponding stdsimd issue #35.

@workingjubilee workingjubilee added the I-nominated We should discuss this at the next weekly meeting label Oct 12, 2020
@workingjubilee workingjubilee removed the I-nominated We should discuss this at the next weekly meeting label Oct 12, 2020
@Lokathor
Copy link
Contributor

Looks good.

The docs should maybe say "whole number" rather than "integer" because these don't convert to an integer type, but that's a small nit that could be fixed later on.

@calebzulawski
Copy link
Member Author

I've usually heard whole numbers to mean the natural numbers (nonnegative integers?) so I think integer is more accurate. But I also just copy-pasted the doc from std 😄

@calebzulawski calebzulawski merged commit 4baa8c2 into master Oct 13, 2020
@calebzulawski calebzulawski deleted the feature/round branch October 13, 2020 03:36
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.

Rounding and Type Change methods
4 participants