Skip to content

Generate bools as bools instead of u8 #809

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 4 commits into from
Nov 30, 2021
Merged

Generate bools as bools instead of u8 #809

merged 4 commits into from
Nov 30, 2021

Conversation

khyperia
Copy link
Contributor

As lengthily discussed in discord.

@khyperia khyperia requested a review from eddyb November 26, 2021 14:12
Comment on lines 791 to 792
// Output. In other words, "stuff the CPU can't see, bools are OK, stuff the CPU can't see, no
// bools". So, we always compile bools to bools, even if they're behind a pointer, and error if
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be:

Suggested change
// Output. In other words, "stuff the CPU can't see, bools are OK, stuff the CPU can't see, no
// bools". So, we always compile bools to bools, even if they're behind a pointer, and error if
// Output. In other words, "stuff the CPU can't see, bools are OK, stuff the CPU can see, no
// bools". So, we always compile bools to bools, even if they're behind a pointer, and error if

?

Copy link
Contributor

Choose a reason for hiding this comment

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

The sentence doesn't quite parse for me

Copy link
Contributor

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Haven't fully checked the code, but it passes a visual inspection, and this seems much nicer.

@@ -126,7 +123,7 @@ pub enum Sampled {
No = 2,
}

#[cfg(any(not(target_arch = "spirv"), target_feature = "Int8"))]
#[cfg(not(target_arch = "spirv"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this needs a cfg at all, like the others

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it needs a cfg.

Copy link
Contributor

@DJMcNab DJMcNab Nov 29, 2021

Choose a reason for hiding this comment

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

My eyes glaze over whilst reading it; I'm still not sure I understand. It looks like it would have been possible to use if Int8 was enabled, but now is not usable on GPU at all.

I'll take your word for it though.

Copy link
Contributor

Choose a reason for hiding this comment

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

but now is not usable on GPU at all.

Correct, Option<bool> breaks with this PR, because it expects bool to be an u8 - even if we forcefully disable the niche, it still turns the bool into u8 as part of a scalar pair (the same (is_some: bool, MaybeUninit<T>)-like scalar pair representation of Option<T> which has allowed for i in 0..n to work, so we can't just mess with it in general, yet).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm - with the bitcast smarts I introduced in #819 this turned out to be rather easy to amend -- will probably add that to the PR, too.

pub fn main(i: u32, o: &mut bool) {
*o = has_two_decimal_digits(i);
pub fn main(i: u32, o: &mut u32) {
*o = if has_two_decimal_digits(i) { 1 } else { 0 };
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I wonder if as u32 works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, cursed, didn't know that existed! yes it does work.

@khyperia khyperia enabled auto-merge (squash) November 30, 2021 12:44
@khyperia khyperia merged commit 2e4de94 into main Nov 30, 2021
@khyperia khyperia deleted the bools-as-bools branch November 30, 2021 13:12
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.

5 participants