-
Notifications
You must be signed in to change notification settings - Fork 52
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
Use Switch Instead of If, main branch (2024.12.04.) #796
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't like this. Not only are switch statements arguably less clear than if-else constructs, but putting the default case at the top, falling through after assertions, etc. are all a no-go as far as I am concerned.
I appreciate that this mimics the original code but let's redesign this properly rather than patching it up with switch
statements. 😕
assert( | ||
"The measurement index out of e_bound_loc0 and " | ||
"e_bound_loc1 should not happen."); | ||
[[fallthrough]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely this should be assert(false, ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately that's not how assert(...)
works. 😦 I think people have used stuff like
assert(false && "Some explanation");
in their codes. (https://stackoverflow.com/questions/3692954/add-custom-messages-in-assert) But this is not super portable either.
You're absolutely right though, that this is an incorrect use of assert(...)
in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right yes, sorry; should be false && ...
; but just putting in a string will ensure that the assertion always succeeds, defeating the point.
assert( | ||
"The measurement index out of e_bound_loc0 and " | ||
"e_bound_loc1 should not happen."); | ||
[[fallthrough]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense for this to fall through to the loc0
case? 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just took the current behaviour, as you've seen as well. Not sure what we should do here instead in an optimised build. 🤔 Generally, I'd like to print an error message. And then either make the kernel stop with an error, or let the kernel continue.
This ties into discussions that we had not long ago about some organised logging infrastructure for our GPU code. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should absolutely change the behaviour! Just as below I believe the correct way to do this is to define a case for loc0
and for loc1
and to mark the default case as unreachable.
matrix_operator().element(ret, 1, 0) = m_measurement.local[1]; | ||
} | ||
switch (m_measurement.subs.get_indices()[0]) { | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this at the top and not at the bottom as is more common in switch statements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be able to fall through it. I thought that was clear.
Indeed, this is not a standard setup. But it seemed like a neat way of achieving the current behaviour.
Whether we want to change the current behaviour is a discussion to be had though.
default: | ||
assert( | ||
"The measurement index out of e_bound_loc0 and " | ||
"e_bound_loc1 should not happen."); | ||
[[fallthrough]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments here.
case 2u: | ||
return update<2u, shape_type>(trk_state, bound_params); | ||
default: | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather finish this with a __builtin_unreachable
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not quite unreachable though, is it? 🤔 In a release build the assertion would let this code be reached. At which point we can once again go back to my previous thoughtline.
- Print an error message;
- Either stop the execution, or let it continue as it currently does.
- Letting it continue, with an error communicated back to the caller about failing the update, does not seem that crazy to me. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course it is not technically unreachable but it is semantically unreachable, which is exactly what is modelled by the assert
. I think it's fair to say that the code reaching this point without D
being either 1 or 2 can be declared undefined behaviour, which is exactly what marking it as unreachable does.
3bff01f
to
c7d26ee
Compare
|
Okay, I added Of course std::unreachable() would be even nicer, but that will be some while away still... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks a lot better already. 😄
case 1u: | ||
result = update<1u, shape_type>(trk_state, bound_params); | ||
break; | ||
case 2u: | ||
result = update<2u, shape_type>(trk_state, bound_params); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case 1u: | |
result = update<1u, shape_type>(trk_state, bound_params); | |
break; | |
case 2u: | |
result = update<2u, shape_type>(trk_state, bound_params); | |
break; | |
case 1u: | |
return update<1u, shape_type>(trk_state, bound_params); | |
case 2u: | |
return update<2u, shape_type>(trk_state, bound_params); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I went with that setup was two-fold:
- I'm not 100% sure what happens with
__builtin_unreachable()
on all the platforms that this code will run on.
I was considering putting
default:
__builtin_unreachable();
return false;
there, "just to be safe". But this formalism seemed better.
- GPU functions can sometimes have hangups about returning at different points. Many coding rules even forbid multiple return points from functions. So I thought this may actually help.
But admittedly, I'm not absolutely sure about either of these points. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy with this! 👍
As a bit of a palette cleanser between larger / messier PRs, I present this simple one. 😄
I came across these
if(...)
blocks while working on the SoA-ification oftraccc::measurement
. 🤔 I think these changes should allow the compiler to generate more efficient code. Though probably not by a whole lot.