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

Prefer not implementing intrinsics in a way that breaks in edge cases #99

Closed
bjorn3 opened this issue Oct 22, 2020 · 0 comments · Fixed by #114
Closed

Prefer not implementing intrinsics in a way that breaks in edge cases #99

bjorn3 opened this issue Oct 22, 2020 · 0 comments · Fixed by #114
Labels
c: rustc_codegen_spirv Issues specific to the rustc_codegen_spirv crate. t: enhancement A new feature or improvement to an existing one.

Comments

@bjorn3
Copy link
Contributor

bjorn3 commented Oct 22, 2020

For example saturating_add isn't implemented in a saturating way:

sym::saturating_add => {
assert_eq!(arg_tys[0], arg_tys[1]);
match &arg_tys[0].kind() {
TyKind::Int(_) | TyKind::Uint(_) => {
self.add(args[0].immediate(), args[1].immediate())
}
TyKind::Float(_) => self.fadd(args[0].immediate(), args[1].immediate()),
other => self.fatal(&format!("Unimplemented intrinsic type: {:#?}", other)),
}
}

This is more of a lesson I learnt the hard way than an issue with rust-gpu itself. While it may seem like a good idea at first to get it quickly working, it will backfire in the long term. I have spent quite a lot of time debugging miscompilations in cg_clif caused by exactly this kind of shortcuts. At minimum I think you should add a FIXME every time you add such a shortcut.

@XAMPPRocky XAMPPRocky added c: rustc_codegen_spirv Issues specific to the rustc_codegen_spirv crate. t: enhancement A new feature or improvement to an existing one. labels Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: rustc_codegen_spirv Issues specific to the rustc_codegen_spirv crate. t: enhancement A new feature or improvement to an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants