You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/05/27 08:50:05 UTC
[GitHub] [tvm] mhyang-pllab opened a new pull request, #11488: [OpenCL] Avoid SelectNode ambiguous overloading
mhyang-pllab opened a new pull request, #11488:
URL: https://github.com/apache/tvm/pull/11488
Consider the SelectNode comes from the test_opencl_ternary_expression test:
```
true_value false_value cond
int8 int8 bool
```
It's was generating the **[OpenCL select function](https://www.khronos.org/registry/OpenCL/sdk/1.0/docs/man/xhtml/select.html)** from the node.
The problem is that [the function signatures are strict](https://github.com/llvm/llvm-project/blob/3606da5fbad042e2b74a35404797af20f65b437b/clang/lib/Headers/opencl-c.h#L11034) and would make it **generate ambiguous calls** with the case already in the testsuite.
According to **[OCL spec 6.3.9. Ternary Selection Operator](https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_C.html#operators-ternary-selection)**, the ternary selection operator is **equivalent to calling with select** but without the difficulty of casting the operands to avoid ambiguous cases.
With this patch, the mentioned test passed. The generated kernels are still equivalent.
Thanks for contributing to TVM! Please refer to guideline https://tvm.apache.org/docs/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @ them in the pull request thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] echuraev commented on pull request #11488: [OpenCL] Avoid SelectNode ambiguous overloading
Posted by GitBox <gi...@apache.org>.
echuraev commented on PR #11488:
URL: https://github.com/apache/tvm/pull/11488#issuecomment-1139719809
Could you also please write types of the arguments which were passed to select operator in case which has failed on your GPU?
Is it `select(half4, half4, ???)` or some other types of arguments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] echuraev commented on a diff in pull request #11488: [OpenCL] Avoid SelectNode ambiguous overloading
Posted by GitBox <gi...@apache.org>.
echuraev commented on code in PR #11488:
URL: https://github.com/apache/tvm/pull/11488#discussion_r883704412
##########
src/target/source/codegen_opencl.cc:
##########
@@ -541,12 +541,26 @@ void CodeGenOpenCL::VisitExpr_(const OrNode* op, std::ostream& os) {
}
void CodeGenOpenCL::VisitExpr_(const SelectNode* op, std::ostream& os) {
+ std::ostringstream oss;
os << "select(";
- PrintExpr(op->false_value, os);
+ PrintExpr(op->false_value, oss);
Review Comment:
I'm not sure, but should we also modify `test_opencl_type_casting` after these changes?
##########
src/target/source/codegen_opencl.cc:
##########
@@ -541,12 +541,26 @@ void CodeGenOpenCL::VisitExpr_(const OrNode* op, std::ostream& os) {
}
void CodeGenOpenCL::VisitExpr_(const SelectNode* op, std::ostream& os) {
+ std::ostringstream oss;
os << "select(";
- PrintExpr(op->false_value, os);
+ PrintExpr(op->false_value, oss);
+ os << CastFromTo(oss.str(), op->false_value.dtype(), op->dtype);
+ oss.str("");
os << ", ";
- PrintExpr(op->true_value, os);
+ PrintExpr(op->true_value, oss);
+ os << CastFromTo(oss.str(), op->true_value.dtype(), op->dtype);
+ oss.str("");
os << ", ";
- PrintExpr(op->condition, os);
+ PrintExpr(op->condition, oss);
+ if (op->dtype.is_float()) {
Review Comment:
I'm not sure, but maybe checking the type of `op->condition` will be enough?
I mean only:
```
if (op->condition.dtype().is_uint() || op->condition.dtype().is_int()) {
os << oss.str();
} else {
os << CastTo(oss.str(), DataType::Int(op->dtype.bits(), op->dtype.lanes()));
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] soto2080 commented on a diff in pull request #11488: [OpenCL] Avoid SelectNode ambiguous overloading
Posted by GitBox <gi...@apache.org>.
soto2080 commented on code in PR #11488:
URL: https://github.com/apache/tvm/pull/11488#discussion_r883763074
##########
src/target/source/codegen_opencl.cc:
##########
@@ -541,12 +541,26 @@ void CodeGenOpenCL::VisitExpr_(const OrNode* op, std::ostream& os) {
}
void CodeGenOpenCL::VisitExpr_(const SelectNode* op, std::ostream& os) {
+ std::ostringstream oss;
os << "select(";
- PrintExpr(op->false_value, os);
+ PrintExpr(op->false_value, oss);
+ os << CastFromTo(oss.str(), op->false_value.dtype(), op->dtype);
+ oss.str("");
os << ", ";
- PrintExpr(op->true_value, os);
+ PrintExpr(op->true_value, oss);
+ os << CastFromTo(oss.str(), op->true_value.dtype(), op->dtype);
+ oss.str("");
os << ", ";
- PrintExpr(op->condition, os);
+ PrintExpr(op->condition, oss);
+ if (op->dtype.is_float()) {
Review Comment:
According to the header, we should only cast the condition operand to int when the return type is a floating point type.
Cond should be casted to the same type with both the true and false operands when not selecting from float.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] echuraev commented on a diff in pull request #11488: [OpenCL] Avoid SelectNode ambiguous overloading
Posted by GitBox <gi...@apache.org>.
echuraev commented on code in PR #11488:
URL: https://github.com/apache/tvm/pull/11488#discussion_r883946708
##########
src/target/source/codegen_opencl.cc:
##########
@@ -541,12 +541,26 @@ void CodeGenOpenCL::VisitExpr_(const OrNode* op, std::ostream& os) {
}
void CodeGenOpenCL::VisitExpr_(const SelectNode* op, std::ostream& os) {
+ std::ostringstream oss;
os << "select(";
- PrintExpr(op->false_value, os);
+ PrintExpr(op->false_value, oss);
+ os << CastFromTo(oss.str(), op->false_value.dtype(), op->dtype);
+ oss.str("");
os << ", ";
- PrintExpr(op->true_value, os);
+ PrintExpr(op->true_value, oss);
+ os << CastFromTo(oss.str(), op->true_value.dtype(), op->dtype);
+ oss.str("");
os << ", ";
- PrintExpr(op->condition, os);
+ PrintExpr(op->condition, oss);
+ if (op->dtype.is_float()) {
Review Comment:
Sorry, my bad. I wasn't careful enough when I read the header file. Thank you for your clarification. I double-checked the header and I agree with you. Now, this if condition is clear enough for me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] echuraev commented on a diff in pull request #11488: [OpenCL] Avoid SelectNode ambiguous overloading
Posted by GitBox <gi...@apache.org>.
echuraev commented on code in PR #11488:
URL: https://github.com/apache/tvm/pull/11488#discussion_r883946708
##########
src/target/source/codegen_opencl.cc:
##########
@@ -541,12 +541,26 @@ void CodeGenOpenCL::VisitExpr_(const OrNode* op, std::ostream& os) {
}
void CodeGenOpenCL::VisitExpr_(const SelectNode* op, std::ostream& os) {
+ std::ostringstream oss;
os << "select(";
- PrintExpr(op->false_value, os);
+ PrintExpr(op->false_value, oss);
+ os << CastFromTo(oss.str(), op->false_value.dtype(), op->dtype);
+ oss.str("");
os << ", ";
- PrintExpr(op->true_value, os);
+ PrintExpr(op->true_value, oss);
+ os << CastFromTo(oss.str(), op->true_value.dtype(), op->dtype);
+ oss.str("");
os << ", ";
- PrintExpr(op->condition, os);
+ PrintExpr(op->condition, oss);
+ if (op->dtype.is_float()) {
Review Comment:
Sorry, my bad. I wasn't careful enough when I read the header file. Thank you for your clarification. I double-checked the header and I agree with you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi merged pull request #11488: [OpenCL] Avoid SelectNode ambiguous overloading
Posted by GitBox <gi...@apache.org>.
masahi merged PR #11488:
URL: https://github.com/apache/tvm/pull/11488
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mhyang-pllab commented on pull request #11488: [OpenCL] Avoid SelectNode ambiguous overloading
Posted by GitBox <gi...@apache.org>.
mhyang-pllab commented on PR #11488:
URL: https://github.com/apache/tvm/pull/11488#issuecomment-1139417581
@echuraev, @masahi please give it a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mhyang-pllab commented on pull request #11488: [OpenCL] Avoid SelectNode ambiguous overloading
Posted by GitBox <gi...@apache.org>.
mhyang-pllab commented on PR #11488:
URL: https://github.com/apache/tvm/pull/11488#issuecomment-1139692975
@echuraev Thank you for your detailed response.
Cast only the true and false operands are not enough here. The condition operand is also required to be casted for resolving the right call.
For any type except float, we can let CastFromTo handle it.
When it's a float type, if the condition is not an int, it's needed to cast to int with the same bits and lanes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mhyang-pllab commented on pull request #11488: [OpenCL] Avoid SelectNode ambiguous overloading
Posted by GitBox <gi...@apache.org>.
mhyang-pllab commented on PR #11488:
URL: https://github.com/apache/tvm/pull/11488#issuecomment-1140177302
@echuraev thanks for your kindly reply.
The purpose of this PR is to fix the falling test_opencl_ternary_expression and try to keep the compatibility with your accurate float4 typecasting case.
You could manually invoke this test then it would fall without this PR.
The test is currently failing with the environment ci_gpu image due to the select function signature causing ambiguous codegen. I don't why the test suite was passed the CI. The codegen problem is also happened in the real-world models.
And I don't know if we should support khr_fp64 and khr_fp16 here. If not so, the current tests are enough to cover the problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] echuraev commented on a diff in pull request #11488: [OpenCL] Avoid SelectNode ambiguous overloading
Posted by GitBox <gi...@apache.org>.
echuraev commented on code in PR #11488:
URL: https://github.com/apache/tvm/pull/11488#discussion_r883489841
##########
src/target/source/codegen_opencl.cc:
##########
@@ -540,16 +540,6 @@ void CodeGenOpenCL::VisitExpr_(const OrNode* op, std::ostream& os) {
os << ")";
}
Review Comment:
```suggestion
void CodeGenOpenCL::VisitExpr_(const SelectNode* op, std::ostream& os) {
std::ostringstream oss;
os << "select(";
PrintExpr(op->false_value, oss);
os << CastFromTo(oss.str(), op->false_value.dtype(), op->dtype);
oss.str("");
os << ", ";
PrintExpr(op->true_value, oss);
os << CastFromTo(oss.str(), op->true_value.dtype(), op->dtype);
oss.str("");
os << ", ";
PrintExpr(op->condition, os);
os << ")";
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] soto2080 commented on a diff in pull request #11488: [OpenCL] Avoid SelectNode ambiguous overloading
Posted by GitBox <gi...@apache.org>.
soto2080 commented on code in PR #11488:
URL: https://github.com/apache/tvm/pull/11488#discussion_r883758786
##########
src/target/source/codegen_opencl.cc:
##########
@@ -541,12 +541,26 @@ void CodeGenOpenCL::VisitExpr_(const OrNode* op, std::ostream& os) {
}
void CodeGenOpenCL::VisitExpr_(const SelectNode* op, std::ostream& os) {
+ std::ostringstream oss;
os << "select(";
- PrintExpr(op->false_value, os);
+ PrintExpr(op->false_value, oss);
Review Comment:
I think your test case should still be right. And I may add other cases later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mhyang-pllab commented on a diff in pull request #11488: [OpenCL] Avoid SelectNode ambiguous overloading
Posted by GitBox <gi...@apache.org>.
mhyang-pllab commented on code in PR #11488:
URL: https://github.com/apache/tvm/pull/11488#discussion_r883765399
##########
src/target/source/codegen_opencl.cc:
##########
@@ -541,12 +541,26 @@ void CodeGenOpenCL::VisitExpr_(const OrNode* op, std::ostream& os) {
}
void CodeGenOpenCL::VisitExpr_(const SelectNode* op, std::ostream& os) {
+ std::ostringstream oss;
os << "select(";
- PrintExpr(op->false_value, os);
+ PrintExpr(op->false_value, oss);
+ os << CastFromTo(oss.str(), op->false_value.dtype(), op->dtype);
+ oss.str("");
os << ", ";
- PrintExpr(op->true_value, os);
+ PrintExpr(op->true_value, oss);
+ os << CastFromTo(oss.str(), op->true_value.dtype(), op->dtype);
+ oss.str("");
os << ", ";
- PrintExpr(op->condition, os);
+ PrintExpr(op->condition, oss);
+ if (op->dtype.is_float()) {
Review Comment:
According to the header, we should only cast the condition operand to int when the return type is a floating point type.
Cond should be casted to the same type with both the true and false operands when not selecting from float.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] echuraev commented on pull request #11488: [OpenCL] Avoid SelectNode ambiguous overloading
Posted by GitBox <gi...@apache.org>.
echuraev commented on PR #11488:
URL: https://github.com/apache/tvm/pull/11488#issuecomment-1140182750
@mhyang-pllab thank you for your reply. I agree with you that the existing test will fail in the CI without your fix. My comment about the test is minor, and shouldn't really affect on the merging of this PR (I already approved this PR). But I think that we could add a tiny test with one select operator and different data types, I think that `float` and `int` will be enough.
But once again, the comment about the test is minor. Please make CI green.
@masahi this PR is LGTM. Please take a look at it and if you agree that we could merge it without a new test, then let's merge it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] echuraev commented on a diff in pull request #11488: [OpenCL] Avoid SelectNode ambiguous overloading
Posted by GitBox <gi...@apache.org>.
echuraev commented on code in PR #11488:
URL: https://github.com/apache/tvm/pull/11488#discussion_r883947644
##########
src/target/source/codegen_opencl.cc:
##########
@@ -541,12 +541,26 @@ void CodeGenOpenCL::VisitExpr_(const OrNode* op, std::ostream& os) {
}
void CodeGenOpenCL::VisitExpr_(const SelectNode* op, std::ostream& os) {
+ std::ostringstream oss;
os << "select(";
- PrintExpr(op->false_value, os);
+ PrintExpr(op->false_value, oss);
Review Comment:
I think it would be nice to have a test which will cover new functionality with adding type casting into select operator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mhyang-pllab commented on a diff in pull request #11488: [OpenCL] Avoid SelectNode ambiguous overloading
Posted by GitBox <gi...@apache.org>.
mhyang-pllab commented on code in PR #11488:
URL: https://github.com/apache/tvm/pull/11488#discussion_r883765298
##########
src/target/source/codegen_opencl.cc:
##########
@@ -541,12 +541,26 @@ void CodeGenOpenCL::VisitExpr_(const OrNode* op, std::ostream& os) {
}
void CodeGenOpenCL::VisitExpr_(const SelectNode* op, std::ostream& os) {
+ std::ostringstream oss;
os << "select(";
- PrintExpr(op->false_value, os);
+ PrintExpr(op->false_value, oss);
Review Comment:
I think your test case should still be right. And I may add other cases later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] soto2080 commented on a diff in pull request #11488: [OpenCL] Avoid SelectNode ambiguous overloading
Posted by GitBox <gi...@apache.org>.
soto2080 commented on code in PR #11488:
URL: https://github.com/apache/tvm/pull/11488#discussion_r883758786
##########
src/target/source/codegen_opencl.cc:
##########
@@ -541,12 +541,26 @@ void CodeGenOpenCL::VisitExpr_(const OrNode* op, std::ostream& os) {
}
void CodeGenOpenCL::VisitExpr_(const SelectNode* op, std::ostream& os) {
+ std::ostringstream oss;
os << "select(";
- PrintExpr(op->false_value, os);
+ PrintExpr(op->false_value, oss);
Review Comment:
I think your test case should still be right. And I may add other cases later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org