Skip to content

Conversation

@edgchen1
Copy link
Contributor

@edgchen1 edgchen1 commented Aug 17, 2020

Description
Check status of BinaryElementwise::Prepare(), UnaryElementwise::Prepare(), and BinaryElementwisePreparation::BinaryElementwiseBroadcastPrepareHelper() and fail on error.

Motivation and Context
Encountered a segmentation fault later on because the result of a failed operation was ignored. Now, at least there is a more helpful error message.

@edgchen1 edgchen1 added the ep:CUDA issues related to the CUDA execution provider label Aug 17, 2020
@edgchen1 edgchen1 requested a review from a team as a code owner August 17, 2020 19:05
snnn
snnn previously approved these changes Aug 17, 2020
thiagocrepaldi
thiagocrepaldi previously approved these changes Aug 17, 2020
SherlockNoMad
SherlockNoMad previously approved these changes Aug 17, 2020
Copy link
Contributor

@SherlockNoMad SherlockNoMad left a comment

Choose a reason for hiding this comment

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

There are a few other places, which used Prepare() and prepare.BinaryElementwiseBroadcastPrepareHelper that didn't handle the error case.

e.g. activation_grad.cc, reduction_ops.cc and complex_mul.cc

Maybe do a global search for fix all all ?

@edgchen1 edgchen1 dismissed stale reviews from SherlockNoMad, thiagocrepaldi, and snnn via cc3bfc6 August 17, 2020 20:42
thiagocrepaldi
thiagocrepaldi previously approved these changes Aug 17, 2020
@edgchen1 edgchen1 changed the title Check status of BinaryElementwise::Prepare(). Check status of element-wise op prepare functions. Aug 17, 2020
@edgchen1 edgchen1 merged commit 32a5f3d into master Aug 17, 2020
@edgchen1 edgchen1 deleted the edgchen1/binary_elementwise_check_status branch August 17, 2020 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ep:CUDA issues related to the CUDA execution provider

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants