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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
