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

[CODEGEN][OPENCL] Fix compile error about ternary expression. #2821

Merged
merged 1 commit into from
Mar 18, 2019

Conversation

lixiaoquan
Copy link
Contributor

Code like this can't be built with NV OpenCL, and it needs an explicit type
converison for ternary expression if return type is uchar.

   uchar i = 0, j = 0;
   uchar t = max((uchar)j, ((i > 0) ? (uchar)1 : (uchar)0));

It is for #2787

@@ -208,6 +208,16 @@ void CodeGenOpenCL::VisitExpr_(const Broadcast* op, std::ostream& os) { // NOL
os << "))";
}

void CodeGenOpenCL::VisitExpr_(const Call *op, std::ostream& os) { // NOLINT(*)
if (op->is_intrinsic(intrinsic::tvm_if_then_else)) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment on why explicit cast is needed here.

Likely you also need to add support for select

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests added for both tvm::if_then_else and Select

@tqchen tqchen added status: need review status: need update need update based on feedbacks status: need RFC need RFC discussion and removed status: need RFC need RFC discussion labels Mar 15, 2019
@@ -208,6 +208,16 @@ void CodeGenOpenCL::VisitExpr_(const Broadcast* op, std::ostream& os) { // NOL
os << "))";
}

void CodeGenOpenCL::VisitExpr_(const Call *op, std::ostream& os) { // NOLINT(*)
if (op->is_intrinsic(intrinsic::tvm_if_then_else)) {
if (op->args[2].type() == UInt(8)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

On my environment, the problem also happens with char, short, and ushort. It looks safe to add a cast for any types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works with float/int, that's why most people didn't run into this problem. Casting for any types is good for me.

@lixiaoquan lixiaoquan force-pushed the opencl branch 2 times, most recently from 7300eba to 580b065 Compare March 16, 2019 06:45
  Code like this can't be built with NV OpenCL, and it needs an explicit type
  converison for ternary expression if return type is uchar.

       uchar i = 0, j = 0;
       uchar t = max((uchar)j, ((i > 0) ? (uchar)1 : (uchar)0));
@kazum kazum removed the status: need update need update based on feedbacks label Mar 18, 2019
@kazum
Copy link
Contributor

kazum commented Mar 18, 2019

@tqchen I think your comment has been addressed. Can you confirm it and update your review status if appropriate?

@kazum kazum merged commit fa70983 into apache:master Mar 18, 2019
@kazum
Copy link
Contributor

kazum commented Mar 18, 2019

Thanks @lixiaoquan @tqchen, this is now merged.

@lixiaoquan lixiaoquan deleted the opencl branch March 19, 2019 00:58
wweic pushed a commit to wweic/tvm that referenced this pull request Mar 20, 2019
…#2821)

Code like this can't be built with NV OpenCL, and it needs an explicit type
  converison for ternary expression if return type is uchar.

       uchar i = 0, j = 0;
       uchar t = max((uchar)j, ((i > 0) ? (uchar)1 : (uchar)0));
wweic pushed a commit to neo-ai/tvm that referenced this pull request Mar 20, 2019
…#2821)

Code like this can't be built with NV OpenCL, and it needs an explicit type
  converison for ternary expression if return type is uchar.

       uchar i = 0, j = 0;
       uchar t = max((uchar)j, ((i > 0) ? (uchar)1 : (uchar)0));
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants