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

Add CUDF_UNREACHABLE macro. #9727

Merged
merged 19 commits into from
Mar 18, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
937a512
Add CUDF_UNREACHABLE macro.
bdice Nov 18, 2021
106b708
Add unreachable macro to type dispatcher.
bdice Nov 18, 2021
f9894d1
Add unreachable macro to AST operator dispatcher.
bdice Nov 18, 2021
8f41a74
Merge remote-tracking branch 'upstream/branch-22.02' into add-cudf-un…
bdice Nov 19, 2021
ab1cdb9
Update CUDF_UNREACHABLE docs.
bdice Nov 19, 2021
77c97ae
Throw error in host-side type dispatcher if invalid type is passed.
bdice Nov 19, 2021
2c39210
Merge remote-tracking branch 'upstream/branch-22.04' into add-cudf-un…
bdice Jan 24, 2022
2d61ea9
Use CUDF_UNREACHABLE only on device in AST operator dispatch.
bdice Jan 24, 2022
a0d9baa
Merge remote-tracking branch 'upstream/branch-22.04' into add-cudf-un…
bdice Jan 26, 2022
8c7d978
Merge remote-tracking branch 'upstream/branch-22.04' into add-cudf-un…
bdice Jan 28, 2022
52ad3a0
Use CUDF_UNREACHABLE (may be too aggressive, some cases may need to f…
bdice Jan 31, 2022
7de2aa0
Merge remote-tracking branch 'upstream/branch-22.04' into add-cudf-un…
bdice Mar 2, 2022
3bcfd60
Replace cudf_assert(false...) in device functions with CUDF_UNREACHABLE.
bdice Mar 2, 2022
c320c1f
Merge remote-tracking branch 'upstream/branch-22.04' into add-cudf-un…
bdice Mar 15, 2022
6d8d498
Merge remote-tracking branch 'upstream/branch-22.04' into add-cudf-un…
bdice Mar 17, 2022
8c2b1dc
Add comment about assert in debug mode.
bdice Mar 17, 2022
1407315
Remove unnecessary returns.
bdice Mar 17, 2022
b66c4c4
Add explicit type since auto deduction fails with CUDF_UNREACHABLE.
bdice Mar 17, 2022
3d3000c
Merge remote-tracking branch 'upstream/branch-22.04' into add-cudf-un…
bdice Mar 18, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions cpp/include/cudf/ast/detail/operators.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -199,13 +199,7 @@ CUDF_HOST_DEVICE inline constexpr void ast_operator_dispatcher(ast_operator op,
case ast_operator::CAST_TO_FLOAT64:
f.template operator()<ast_operator::CAST_TO_FLOAT64>(std::forward<Ts>(args)...);
break;
default:
#ifndef __CUDA_ARCH__
CUDF_FAIL("Invalid operator.");
#else
cudf_assert(false && "Invalid operator.");
#endif
break;
default: CUDF_UNREACHABLE("Unsupported operator.");
bdice marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
23 changes: 23 additions & 0 deletions cpp/include/cudf/detail/utilities/assert.cuh
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,26 @@
#else
#define cudf_assert(e) (static_cast<void>(0))
#endif

/**
* @brief Macro indicating that a location in the code is unreachable.
*
* The CUDF_UNREACHABLE macro should only be used where CUDF_FAIL cannot be used
* due to performance or due to being used in device code. In the majority of
* host code situations, an exception should be thrown in "unreachable" code
* paths as those usually aren't tight inner loops like they are in device code.
*
* One example where this macro may be used is in conjunction with dispatchers
* to indicate that a function does not need to return a default value because
* it has already exhausted all possible cases in a `switch` statement.
*
* Example usage:
* ```
* CUDF_UNREACHABLE("Invalid type_id.");
* ```
*/
#define CUDF_UNREACHABLE(msg) \
do { \
assert(false && "Unreachable: " msg); \
vuule marked this conversation as resolved.
Show resolved Hide resolved
__builtin_unreachable(); \
} while (0)
13 changes: 2 additions & 11 deletions cpp/include/cudf/utilities/type_dispatcher.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -511,18 +511,9 @@ CUDF_HOST_DEVICE __forceinline__ constexpr decltype(auto) type_dispatcher(cudf::
std::forward<Ts>(args)...);
default: {
#ifndef __CUDA_ARCH__
CUDF_FAIL("Unsupported type_id.");
CUDF_FAIL("Invalid type_id.");
#else
cudf_assert(false && "Unsupported type_id.");

// The following code will never be reached, but the compiler generates a
// warning if there isn't a return value.

// Need to find out what the return type is in order to have a default
// return value and solve the compiler warning for lack of a default
// return
using return_type = decltype(f.template operator()<int8_t>(std::forward<Ts>(args)...));
return return_type();
CUDF_UNREACHABLE("Invalid type_id.");
#endif
Comment on lines 513 to 517
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be useful to make this into a single macro (maybe this should be CUDF_UNREACHABLE, so it covers both host and device code)? I see the pattern in a few places in the PR.

Copy link
Contributor Author

@bdice bdice Mar 17, 2022

Choose a reason for hiding this comment

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

I considered that, but I didn't want to hide the dependence on #ifndef __CUDA_ARCH__. Failure/raising an error and unreachable code mean very different things in my opinion, and I didn't want to conflate them by replacing this with an idiom that has potential for misuse. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure. It's weird because we do have the uneven handling between host and device as it is. Maybe it should be the other way around, and CUDF_FAIL can call CUDF_UNREACHABLE if in device code. As in - "we failed on the device, here's an assert if debug and don't expect a return".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tagging @jrhemstad for thoughts on this. I would defer that change to a later PR if possible.

Copy link
Contributor Author

@bdice bdice Mar 17, 2022

Choose a reason for hiding this comment

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

I think I'm still in favor of keeping these macros separate. Letting CUDF_FAIL defer to an unreachable path seems dangerous. Developers that see CUDF_FAIL should be able to reasonably expect an error, and should not use it to signify branches that can be optimized out as impossible to reach. A macro named something like CUDF_IMPOSSIBLE might be a compromise, but I think a combined macro like that would obscure the intention (in harmful ways) more than it helps with cleanliness/brevity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, obscuring the intention is the main issue I can see.

Here's what bugs me: we are using CUDF_UNREACHABLE both for truly unreachable code and failure. Ideally, CUDF_UNREACHABLE macro would call GCC's __builtin_unreachable() if in host code. But we call CUDF_FAIL instead in such cases.
Feels like code that should not be executed should use CUDF_FAIL (both host and device) and truly unreachable code should use CUDF_UNREACHABLE (both host and device). I understand that this may do more hard than good, just bringing it up for consideration.

Copy link
Contributor Author

@bdice bdice Mar 17, 2022

Choose a reason for hiding this comment

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

I believe all the cases handled in this way are actually unreachable (by enum exhaustion, in most cases). We’re just taking the opportunity to raise an error on the host because we can do that without any significant performance or compile time penalty.

}
}
Expand Down