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

.abs() and .signum() cause 64-bit integer instruction generation #468

Closed
hrydgard opened this issue Mar 4, 2021 · 7 comments
Closed

.abs() and .signum() cause 64-bit integer instruction generation #468

hrydgard opened this issue Mar 4, 2021 · 7 comments
Labels
t: bug Something isn't working t: external Issues not about rust-gpu itself, but related enough to be tracked.

Comments

@hrydgard
Copy link
Contributor

hrydgard commented Mar 4, 2021

This is arguably not rust-gpu's fault, but num-traits, but it'll happen again, so we need a record of it.

Using vector abs ends up generating 64-bit ops due to https://github.com/rust-num/num-traits/blob/47d69223ce7707ebbf8a91c3e6b3997e48653feb/src/float.rs#L762 .

Also see the following image:

image

Ideally of course, abs should just generate a SPIR-V abs instruction instead of doing bithacks.

@hrydgard hrydgard added the t: bug Something isn't working label Mar 4, 2021
@khyperia
Copy link
Contributor

Just adding here that the error reporting system for this is absolutely horrible right now, and should be improved (the way I got that screenshot was hacking in random stuff to the compiler to add error points to get a backtrace to figure out what was wrong, see the nyaah error message in the screenshot, haha).

The way that would be done is implementing the capability computation system @XAMPPRocky came up with, tracked in #42 (I'll add the system to a comment there)

@hrydgard
Copy link
Contributor Author

hrydgard commented Apr 19, 2021

Another affected function is signum, likely also copysign.

@hrydgard hrydgard changed the title abs causes 64-bit integer instruction generation .abs() and .signum() cause 64-bit integer instruction generation Apr 22, 2021
@khyperia
Copy link
Contributor

Ah, never actually wrote this down, .abs() is fixed by rust-num/num-traits#196, and I don't believe copysign exists in num-traits for some reason. signum is still broken, though, going to see if I can PR a fix now. However, rust-num/num-traits#196 isn't in any num-traits release yet, so it's still broken for us.

@XAMPPRocky
Copy link
Member

XAMPPRocky commented Jun 14, 2021

…and I don't believe copysign exists in num-traits for some reason.

@khyperia I opened a PR for it awhile ago rust-num/num-traits#207. I've sent them a friendly ping to look at it again.

@khyperia
Copy link
Contributor

@XAMPPRocky are you still planning on getting rust-num/num-traits#207 in?

@XAMPPRocky
Copy link
Member

@khyperia Just made the final needed changes there, but need to wait for the CI run to be approved first to check.

@khyperia khyperia added the t: external Issues not about rust-gpu itself, but related enough to be tracked. label Dec 8, 2021
@oisyn
Copy link
Contributor

oisyn commented Jul 13, 2022

num-traits issue was merged into main on May 3. Everything seems to work as expected now.

@oisyn oisyn closed this as completed Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: bug Something isn't working t: external Issues not about rust-gpu itself, but related enough to be tracked.
Projects
None yet
Development

No branches or pull requests

4 participants