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/11/02 16:39:04 UTC
[GitHub] [tvm] cconvey opened a new pull request, #13267: [build][tir] suppress -Woverloaded-virtual warning
cconvey opened a new pull request, #13267:
URL: https://github.com/apache/tvm/pull/13267
Suppress a spurious `-Woverloaded-virtual` clang warning for the `tvm::tir::DataTypeRewriter` class.
It appears that clang has supported this version for a long time, so I don't think we're safe to ignore clang version for this pragma.
--
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] cconvey commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013408199
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
Sorry it took several examples to make my point :)
> I think it's common in the codebase to only override visitors for some IR nodes, for example https://github.com/apache/tvm/blob/main/src/tir/transforms/storage_rewrite.cc#L63. Any idea how it works?
I think we'd need to look more closely at code that _calls_ one of those methods to see what's going on. Two possibilities that come to mind are:
(a) We're doing something equivalent to this:
``` c++
StmtExprVisitor * p = new LinearAccessPatternFinder(...);
p->VisitStmt_(...)
```
Which at _compile time_ will search the `VisitStmt_` overloads provided by `StmtExprVisitor`. (Although compile-time dispatch will still use the vtable, and thus resolve to a suitable `LinearAccessPatternFinder method if it exists.)
(b) We're unwittingly invoking the wrong overload provided by `LinearAccessPatternFinder` because C++ can automatically cast the _argument_ to one of the overloads provided by `LinearAccessPatternFinder`. And TVM's testing just never noticed the mistake.
--
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] cconvey commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013385933
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
Sorry, let me try making my point better with a slightly different example:
``` c++
#include <iostream>
#include <string>
using namespace std;
class BaseClass {
public:
virtual void foo(int x) {
cout << "BaseClass::foo(int): x = " << x << endl;
}
virtual void foo(string x) {
cout << "BaseClass::foo(string): x = " << x << endl;
}
};
class DerivedClass : public BaseClass {
public:
#ifdef OVERLOAD_FOO
virtual void foo(int x) {
cout << "DerivedClass::foo(int): x = " << x << endl;
}
#endif
};
int main() {
DerivedClass d;
d.foo("Hello, TVM!");
}
```
If I compile this with simply `clang++ x.cpp`, the compilation succeeds. And when I run the resulting executable, it prints `BaseClass::foo(string): x = Hello, TVM!`.
But if I try to compile this with `clang++ -DOVERLOAD_FOO x.cpp`, I get this error:
```
x.cpp:27:11: error: cannot initialize a parameter of type 'int' with an lvalue of type 'const char[12]'
d.foo("Hello, TVM!");
^~~~~~~~~~~~~
x.cpp:19:26: note: passing argument to parameter 'x' here
virtual void foo(int x) {
^
1 error generated.
```
The issue, as I understand it, is that because `DerviedClass` provides _even one_ override of `foo`, C++ will no longer look at _any_ of the `BaseClass::foo(...)` overloads when it sees a call such as `d.foo("Hello, TVM!")` in the example above.
Those base-class functions aren't _completely_ hidden. For example, if we use `d.BaseClass("Hello, TVM!")` instead, the code compiles just fine.
--
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] cconvey commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013408199
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
Sorry it took several examples to make my point :)
> I think it's common in the codebase to only override visitors for some IR nodes, for example https://github.com/apache/tvm/blob/main/src/tir/transforms/storage_rewrite.cc#L63. Any idea how it works?
I think we'd need to look more closely at code that _calls_ one of those methods to see what's going on. Two possibilities that come to mind are:
(a) We're doing something equivalent to this:
``` c++
StmtExprVisitor * p = new LinearAccessPatternFinder(...);
p->VisitStmt_(...)
```
Which at _compile time_ will search the `VisitStmt_` overloads provided by `StmtExprVisitor`. (Although compile-time dispatch will still use the vtable, and thus resolve to a suitable `LinearAccessPatternFinder method if it exists.)
If only code in one of the _ancestor_ classes directly calls `VisitStmt_`, then maybe we could address this by making `VisitStmt_` be `protected` rather than `public`? That might at least _reduce_ the opportunities for this to bite people, although @Lunderberg 's `using ...` suggestion is even more robust AFAICT.
(b) We're unwittingly invoking the wrong overload provided by `LinearAccessPatternFinder` because C++ can automatically cast the _argument_ to one of the overloads provided by `LinearAccessPatternFinder`. And TVM's testing just never noticed the mistake. (So, similar to what happens in that first example I gave, above: https://github.com/apache/tvm/pull/13267#discussion_r1013363645)
--
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] cconvey commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013363645
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
I don't think adding more overloads to the parent class changes the issue. Here's how I understand it (but I may be missing something...)
Suppose we have code like this:
``` c++
#include <iostream>
using namespace std;
class BaseClass {
public:
virtual void foo(int x) {
cout << "BaseClass::foo(int): x = " << x << endl;
}
};
class DerivedClass : public BaseClass {
public:
virtual void foo(float x) {
cout << "DerivedClass::foo(float): x = " << x << endl;
}
};
int main() {
DerivedClass d;
d.foo(42);
}
```
Many people would expect this to print `BaseClass::foo(int): x = 42` because `42` is an int.
But what _actually_ gets printed is `DerivedClass::foo(float): x = 42`. This is unintuitive for a lot of C++ programmers, which I think is why this warning was created.
IIUC, it adding the `using ...` statements that @Lunderberg proposed would make this code behave more like you intended originally.
--
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] cconvey commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013326232
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
Good point. Maybe it should hinge on what the author (@vinx13) had in mind here w.r.t. the base-class overloads: hide them, or don't care?
@vinx13 : Can you clarify?
--
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] cconvey commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013385933
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
Sorry, let me try making my point better with a slightly different example:
``` c++
#include <iostream>
#include <string>
using namespace std;
class BaseClass {
public:
virtual void foo(int x) {
cout << "BaseClass::foo(int): x = " << x << endl;
}
virtual void foo(string x) {
cout << "BaseClass::foo(string): x = " << x << endl;
}
};
class DerivedClass : public BaseClass {
public:
#ifdef OVERLOAD_FOO
virtual void foo(int x) {
cout << "DerivedClass::foo(int): x = " << x << endl;
}
#endif
};
int main() {
DerivedClass d;
d.foo("Hello, TVM!");
}
```
If I compile this with simply `clang++ x.cpp`, the compilation succeeds. And when I run the resulting executable, it prints `BaseClass::foo(string): x = Hello, TVM!`.
But if I try to compile this with `clang++ -DOVERLOAD_FOO x.cpp`, I get this error:
```
x.cpp:27:11: error: cannot initialize a parameter of type 'int' with an lvalue of type 'const char[12]'
d.foo("Hello, TVM!");
^~~~~~~~~~~~~
x.cpp:19:26: note: passing argument to parameter 'x' here
virtual void foo(int x) {
^
1 error generated.
```
The issue, as I understand it, is that because `DerviedClass` provides _even one_ override of `foo`, C++ will no longer look at _any_ of the `BaseClass::foo(...)` overloads when it sees a call such as `d.foo("Hello, TVM!")` in the example above.
(However, those base-class functions aren't _completely_ hidden. For example, if we use `d.BaseClass("Hello, TVM!")` instead, the code compiles just fine.)
--
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] cconvey commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013408199
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
Sorry it took several examples to make my point :)
> I think it's common in the codebase to only override visitors for some IR nodes, for example https://github.com/apache/tvm/blob/main/src/tir/transforms/storage_rewrite.cc#L63. Any idea how it works?
I think we'd need to look more closely at code that _calls_ one of those methods to see what's going on. Two possibilities that come to mind are:
(a) We're doing something equivalent to this:
``` c++
StmtExprVisitor * p = new LinearAccessPatternFinder(...);
p->VisitStmt_(...)
```
Which at _compile time_ will search the `VisitStmt_` overloads provided by `StmtExprVisitor`. (Although runtime dispatch will still use the vtable, and thus resolve to a suitable `LinearAccessPatternFinder method if it exists.)
If only code in one of the _ancestor_ classes directly calls `VisitStmt_`, then maybe we could address this by making `VisitStmt_` be `protected` rather than `public`? That might at least _reduce_ the opportunities for this to bite people, although @Lunderberg 's `using ...` suggestion is even more robust AFAICT.
(b) We're unwittingly invoking the wrong overload provided by `LinearAccessPatternFinder` because C++ can automatically cast the _argument_ to one of the overloads provided by `LinearAccessPatternFinder`. And TVM's testing just never noticed the mistake. (So, similar to what happens in that first example I gave, above: https://github.com/apache/tvm/pull/13267#discussion_r1013363645)
--
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] tvm-bot commented on pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
tvm-bot commented on PR #13267:
URL: https://github.com/apache/tvm/pull/13267#issuecomment-1300861893
<!---bot-comment-->
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from [Reviewers](https://github.com/apache/incubator-tvm/blob/master/CONTRIBUTORS.md#reviewers) by @-ing them in a comment.
<!--bot-comment-ccs-start-->
* cc @Hzfengsy, @junrushao <sub>See [#10317](https://github.com/apache/tvm/issues/10317) for details</sub><!--bot-comment-ccs-end-->
<sub>Generated by [tvm-bot](https://github.com/apache/tvm/blob/main/ci/README.md#github-actions)</sub>
--
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] vinx13 commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
vinx13 commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013391296
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
Thanks for the explanation. I think it's common in the codebase to only override visitors for some IR nodes, for example https://github.com/apache/tvm/blob/main/src/tir/transforms/storage_rewrite.cc#L63. Any idea how it works?
--
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] cconvey commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013385933
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
Sorry, let me try making my point better with a slightly different example:
``` c++
#include <iostream>
#include <string>
using namespace std;
class BaseClass {
public:
virtual void foo(int x) {
cout << "BaseClass::foo(int): x = " << x << endl;
}
virtual void foo(string x) {
cout << "BaseClass::foo(string): x = " << x << endl;
}
};
class DerivedClass : public BaseClass {
public:
#ifdef OVERLOAD_FOO
virtual void foo(int x) {
cout << "DerivedClass::foo(int): x = " << x << endl;
}
#endif
};
int main() {
DerivedClass d;
d.foo("Hello, TVM!");
}
```
If I compile this with simply `clang++ x.cpp`, the compilation succeeds. And when I run the resulting executable, it prints `BaseClass::foo(string): x = Hello, TVM!`.
But if I try to compile this with `clang++ -DOVERLOAD_FOO x.cpp`, I get this error:
```
x.cpp:27:11: error: cannot initialize a parameter of type 'int' with an lvalue of type 'const char[12]'
d.foo("Hello, TVM!");
^~~~~~~~~~~~~
x.cpp:19:26: note: passing argument to parameter 'x' here
virtual void foo(int x) {
^
1 error generated.
```
The issue, as I understand it, is that because `DerviedClass` provides _even one_ override of a base-class virtual function (`foo`), C++ will no longer look at _any_ of the `BaseClass::foo(...)` overloads when it sees a call such as `d.foo("Hello, TVM!")` in the example above.
Those base-class functions aren't _completely_ hidden. For example, if we use `d.BaseClass("Hello, TVM!")` instead, the code compiles just fine.
--
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] cconvey commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013385933
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
Sorry, let me try making my point better with a slightly different example:
``` c++
#include <iostream>
#include <string>
using namespace std;
class BaseClass {
public:
virtual void foo(int x) {
cout << "BaseClass::foo(int): x = " << x << endl;
}
virtual void foo(string x) {
cout << "BaseClass::foo(string): x = " << x << endl;
}
};
class DerivedClass : public BaseClass {
public:
#ifdef OVERLOAD_FOO
virtual void foo(int x) {
cout << "DerivedClass::foo(int): x = " << x << endl;
}
#endif
};
int main() {
DerivedClass d;
d.foo("Hello, TVM!");
}
```
If I compile this with simply `clang++ x.cpp`, the compilation succeeds. And when I run the resulting executable, it prints `BaseClass::foo(string): x = Hello, TVM!`.
But if I try to compile this with `clang++ -DOVERLOAD_FOO x.cpp`, I get this error:
```
x.cpp:27:11: error: cannot initialize a parameter of type 'int' with an lvalue of type 'const char[12]'
d.foo("Hello, TVM!");
^~~~~~~~~~~~~
x.cpp:19:26: note: passing argument to parameter 'x' here
virtual void foo(int x) {
^
1 error generated.
```
The issue, as I understand it, is that because `DerviedClass` provides _even one_ override of `foo`, C++ will no longer look at _any_ of the `BaseClass::foo(...)` overloads when it sees a call such as `d.foo("Hello, TVM!")` in the example above.
(However, those base-class functions aren't _completely_ hidden. For example, if we use `d.BaseClass::foo("Hello, TVM!")` instead, the code compiles just fine.)
--
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] vinx13 commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
vinx13 commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013331112
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
It only overrides some methods, for the rest it is intended to go to its parent class. I already add https://github.com/apache/tvm/blob/main/include/tvm/tir/stmt_functor.h#L530-L531 to its parent class, not sure why the warning still occurs.
--
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] cconvey commented on pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on PR #13267:
URL: https://github.com/apache/tvm/pull/13267#issuecomment-1303534616
> Can we apply a similar change in the [DataTypeLegalizer](https://github.com/apache/tvm/blob/main/include/tvm/tir/stmt_functor.h#L506)? (Either as part of this PR or another.) It has the same issue of the VisitStmt_ methods being exposed, when external users should go through StmtExprMutator::operator() instead.
Thanks for the idea! It's not too much scope creep, so I'll add that to this PR.
--
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] vinx13 merged pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
vinx13 merged PR #13267:
URL: https://github.com/apache/tvm/pull/13267
--
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] vinx13 commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
vinx13 commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013375916
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
I expect it to only override virtual methods already available in `DataTypeLegalizer` or `StmtExprMutator`, instead of adding new overloads, maybe I'm missing something
--
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] Lunderberg commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013504662
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
(A little bit late to the discussion, but I hope this helps.)
I looked a bit into the warning, and it looks like part of it is because the `DataTypeLegalizer` defines the `VisitStmt_` as public, whereas the `StmtExprMutator` defines them as protected. So the other instances of this pattern are overriding a protected method, which doesn't trigger the warning.
As example code. the warning is trying to avoid cases like the third one below, where the static type causes an unexpected overload set when calling a method on the derived type.
```c++
class Base {
public:
virtual func(int x) {}
virtual func(double x) {}
};
class Derived {
public:
virtual func(double x) {}
};
int main() {
// Static type is base, uses Base's vtable.
std::unique_ptr<Base> base = std::make_unique<Base>();
base->func(5); // Calls Base::func(int)
// Static type is base, uses Derived's vtable.
std::unique_ptr<Base> derived = std::make_unique<Dervied>();
derived->func(5); // Calls Base::func(int)
// Static type is Derived, uses Derived's vtable. Overload
// resolution selects `Derived::func(double)` because the overload
// resolution set is based on the static type, and doesn't include
// `Base::func(int)`.
std::unique_ptr<Derived> derived = std::make_unique<Dervied>();
derived->func(5); // Calls Derived::func(double)
}
```
--
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] cconvey commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013408199
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
Sorry it took several examples to make my point :)
> I think it's common in the codebase to only override visitors for some IR nodes, for example https://github.com/apache/tvm/blob/main/src/tir/transforms/storage_rewrite.cc#L63. Any idea how it works?
I think we'd need to look more closely at code that _calls_ one of those methods to see what's going on. Two possibilities that come to mind are:
(a) We're doing something equivalent to this:
``` c++
StmtExprVisitor * p = new LinearAccessPatternFinder(...);
p->VisitStmt_(...)
```
Which at _compile time_ will search the `VisitStmt_` overloads provided by `StmtExprVisitor`. (Although compile-time dispatch will still use the vtable, and thus resolve to a suitable `LinearAccessPatternFinder method if it exists.)
(b) We're unwittingly invoking the wrong overload provided by `LinearAccessPatternFinder` because C++ can automatically cast the _argument_ to one of the overloads provided by `LinearAccessPatternFinder`. And TVM's testing just never noticed the mistake. (So, similar to what happens in that first example I gave, above: https://github.com/apache/tvm/pull/13267#discussion_r1013363645)
--
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] Lunderberg commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013209580
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
Instead of suppressing the warning altogether, could we instead add a `using Parent::VisitStmt_;` and `using Parent::VisitExpr_`? That would allow the parent's virtual functions to participate in overload resolution, avoiding the case that the warning is trying to catch.
--
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] cconvey commented on pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on PR #13267:
URL: https://github.com/apache/tvm/pull/13267#issuecomment-1303316269
@vinx13 : I've updated the PR to mark the new methods as `protected`, because IIUC that's more in line with your original intentions.
I left the `#pragma`s in place, because using `protected` wasn't sufficient to make the warning go away.
--
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] cconvey commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013363645
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
I don't think adding more overloads to the parent class changes the issue. Here's how I understand it (but I may be missing something...)
Suppose we have code like this:
``` c++
#include <iostream>
using namespace std;
class BaseClass {
public:
virtual void foo(int x) {
cout << "BaseClass::foo(int): x = " << x << endl;
}
};
class DerivedClass : public BaseClass {
public:
virtual void foo(float x) {
cout << "DerivedClass::foo(float): x = " << x << endl;
}
};
int main() {
DerivedClass d;
d.foo(42);
}
```
Many people would expect this to print `BaseClass::foo(int): x = 42` because `42` is an int.
But what _actually_ gets printed is `DerivedClass::foo(float): x = 42`. This is unintuitive for a lot of C++ programmers, which I think is why this warning was created.
IIUC, adding the `using ...` statements that @Lunderberg proposed would make this code behave more like you intended originally.
--
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] cconvey commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013408199
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
Sorry it took several examples to make my point :)
> I think it's common in the codebase to only override visitors for some IR nodes, for example https://github.com/apache/tvm/blob/main/src/tir/transforms/storage_rewrite.cc#L63. Any idea how it works?
I think we'd need to look more closely at code that _calls_ one of those methods to see what's going on. Two possibilities that come to mind are:
(a) We're doing something equivalent to this:
``` c++
StmtExprVisitor * p = new LinearAccessPatternFinder(...);
p->VisitStmt_(...)
```
Which at _compile time_ will search the `VisitStmt_` overloads provided by `StmtExprVisitor`. (Although compile-time dispatch will still use the vtable, and thus resolve to a suitable `LinearAccessPatternFinder method if it exists.)
If only code in one of the _ancestor_ classes directly calls `VisitStmt_`, then maybe we could address this by making `VisitStmt_` be `protected` rather than `public`?
(b) We're unwittingly invoking the wrong overload provided by `LinearAccessPatternFinder` because C++ can automatically cast the _argument_ to one of the overloads provided by `LinearAccessPatternFinder`. And TVM's testing just never noticed the mistake. (So, similar to what happens in that first example I gave, above: https://github.com/apache/tvm/pull/13267#discussion_r1013363645)
--
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] cconvey commented on pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on PR #13267:
URL: https://github.com/apache/tvm/pull/13267#issuecomment-1303347884
I've reworked this based on our discussion:
- The warnings are eliminated via `using` statements, rather than pragmas.
- I changed the methods from `public` to `protected`, to better match (AFAICT) @vinx13 's intent.
@Lunderberg @vinx13 : Ready for review imho.
--
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] cconvey commented on pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on PR #13267:
URL: https://github.com/apache/tvm/pull/13267#issuecomment-1301452879
CC: @Lunderberg
--
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] cconvey commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013385933
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
Sorry, let me try making my point better with a slightly different example:
``` c++
#include <iostream>
#include <string>
using namespace std;
class BaseClass {
public:
virtual void foo(int x) {
cout << "BaseClass::foo(int): x = " << x << endl;
}
virtual void foo(string x) {
cout << "BaseClass::foo(string): x = " << x << endl;
}
};
class DerivedClass : public BaseClass {
public:
#ifdef OVERLOAD_FOO
virtual void foo(int x) {
cout << "DerivedClass::foo(int): x = " << x << endl;
}
#endif
};
int main() {
DerivedClass d;
d.foo("Hello, TVM!");
}
```
If I compile this with simply `clang++ -Woverloaded-virtual x.cpp`, the compilation succeeds. And when I run the resulting executable, it prints `BaseClass::foo(string): x = Hello, TVM!`.
But if I try to compile this with `clang++ -Woverloaded-virtual -DOVERLOAD_FOO x.cpp`, I get this error:
```
x.cpp:27:11: error: cannot initialize a parameter of type 'int' with an lvalue of type 'const char[12]'
d.foo("Hello, TVM!");
^~~~~~~~~~~~~
x.cpp:19:26: note: passing argument to parameter 'x' here
virtual void foo(int x) {
^
1 error generated.
```
The issue, as I understand it, is that because `DerviedClass` provides _even one_ override of a base-class virtual function (`foo`), C++ will no longer look at _any_ of the `BaseClass::foo(...)` overloads when it sees a call such as `d.foo("Hello, TVM!")` in the example above.
Those base-class functions aren't _completely_ hidden. For example, if we use `d.BaseClass("Hello, TVM!")` instead, the code compiles just fine.
--
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] cconvey commented on a diff in pull request #13267: [build][tir] suppress -Woverloaded-virtual warning
Posted by GitBox <gi...@apache.org>.
cconvey commented on code in PR #13267:
URL: https://github.com/apache/tvm/pull/13267#discussion_r1013385933
##########
src/tir/transforms/narrow_datatype.cc:
##########
@@ -187,6 +187,11 @@ class DataTypeVisitor final : public StmtExprVisitor {
arith::ConstIntBoundAnalyzer::BoundMapType bound_;
};
+#if __clang__
Review Comment:
Sorry, let me try making my point better with a slightly different example:
``` c++
#include <iostream>
#include <string>
using namespace std;
class BaseClass {
public:
virtual void foo(int x) {
cout << "BaseClass::foo(int): x = " << x << endl;
}
virtual void foo(string x) {
cout << "BaseClass::foo(string): x = " << x << endl;
}
};
class DerivedClass : public BaseClass {
public:
#ifdef OVERLOAD_FOO
virtual void foo(int x) {
cout << "DerivedClass::foo(int): x = " << x << endl;
}
#endif
};
int main() {
DerivedClass d;
d.foo("Hello, TVM!");
}
```
If I compile this with simply `clang++ -Woverloaded-virtual x.cpp`, the compilation succeeds. When I run the resulting executable, it prints `BaseClass::foo(string): x = Hello, TVM!`.
If I try to compile this with `clang++ -Woverloaded-virtual -DOVERLOAD_FOO x.cpp`, I get this error:
```
x.cpp:27:11: error: cannot initialize a parameter of type 'int' with an lvalue of type 'const char[12]'
d.foo("Hello, TVM!");
^~~~~~~~~~~~~
x.cpp:19:26: note: passing argument to parameter 'x' here
virtual void foo(int x) {
^
1 error generated.
```
The issue, as I understand it, is that because `DerviedClass` provides _even one_ override of a base-class virtual function (`foo`), C++ will no longer look at _any_ of the `BaseClass::foo(...)` overloads when it sees a call such as `d.foo("Hello, TVM!")` in the example above.
Those base-class functions aren't _completely_ hidden. For example, if we use `d.BaseClass("Hello, TVM!")` instead, the code compiles just fine.
--
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