[build][tir] suppress -Woverloaded-virtual warning

[cconvey]

Suppress a spurious -Woverloaded-virtual clang warning for the tvm::tir::DataTypeRewriter class.

I think we're safe to ignore clang version for this pragma, because it's been supported for a long time now.

[tvm-bot]

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 by @-ing them in a comment.

• cc @Hzfengsy, @junrushao _{See #10317 for details}
• Built docs for commit 76a2b71 can be found here.

_{Generated by tvm-bot}

[cconvey]

CC: @Lunderberg

[Lunderberg]

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.

[cconvey]

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?

[vinx13]

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.

[cconvey]

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:

#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.

[vinx13]

I expect it to only override virtual methods already available in DataTypeLegalizer or StmtExprMutator, instead of adding new overloads, maybe I'm missing something

[cconvey]

Sorry, let me try making my point better with a slightly different example:

#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.)

[vinx13]

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?

[cconvey]

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:

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: #13267 (comment))

[Lunderberg]

(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.

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)
}

[cconvey]

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.

[Lunderberg]

LGTM! Can we apply a similar change in the DataTypeLegalizer? (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.

[cconvey]

Can we apply a similar change in the DataTypeLegalizer? (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.