[CIR][MLIR] Fix overloaded virtual warnings #1439
Conversation
There is a difference in behavior between clang and gcc that was leading to a very large number of -Woverloaded-virtual warnings when compiling with gcc. The difference in behavior is that gcc warns for "partial" virtual overloads. That is, when a base class contains multiple virtual functions with the same name and a derived class overrides at least one but not all of them, gcc issues a warning saying that the non-overridden variations were hidden. This diagnostic is only reported if the -Woverloaded-virtual is explicitly specified on the command line, which it is by the clang project. The standard way to suppress this warning, indicating that the behavior was intentional (which it was in all cases I'm fixing here) is to add a "using" statement for the overloaded symbol in the derived class. However, there was a secondary problem that this couldn't be done in the ClangIR classes because of a "using" clause in the base class that was marked as "private". This change updates a few files in the MLIR, both to allow subclasses to add their own "using" statements by making the base class "using" statement protected, and also fixes a large number of places where the derived class "using" statement was needed in MLIR dialects, in addition to adding such statements where needed in the ClangIR code. The MLIR changes will likely be made upstream, but I am adding them here in the clangir incubator as an proof-of-concept to show what will be needed in downstream projects.
|
Can we split this PR int one for MLIR and another one for CIR? But approach LGTM! |
| @@ -13,6 +13,7 @@ | |||
|
|
|||
| #include "mlir/Conversion/VectorToSCF/VectorToSCF.h" | |||
| #include "mlir/Dialect/Bufferization/IR/Bufferization.h" | |||
| #include "mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.h" | |||
There was a problem hiding this comment.
I'm not sure why this happened. I think my editor did it when I wasn't looking.
I can do that, but I'm not sure I understand the benefit of doing so in the incubator. I think that if the MLIR community doesn't want to make this change upstream we'll need to do something different in the CIR code (like maybe adding Will splitting them up making rebasing easier? |
Potentially yes, should have mentioned in the other comment! That's the solely the reason |
No problem in having a custom solution if they don't want this! |
| @@ -1415,6 +1416,7 @@ struct DownscaleSizeOneWindowed2DConvolution final | |||
| PatternRewriter &rewriter) const override { | |||
| return returningMatchAndRewrite(convOp, rewriter); | |||
| } | |||
| using OpRewritePattern<Conv2DOp>::matchAndRewrite; | |||
There was a problem hiding this comment.
Wouldn't it be better to have these protected like in all other places?
There was a problem hiding this comment.
Honestly, I think we at least don't care that the other variants are hidden, and don't necessarily want them here public or protected. They're public in the base class, I believe, but I don't suppose we want them to be called, so probably protected is better.
Or maybe this is an argument for just adding -Wno-overloaded-virtual to the ClangIR build, since apart from the warning the current behavior is what we want.
There was a problem hiding this comment.
Or maybe this is an argument for just adding -Wno-overloaded-virtual to the ClangIR build, since apart from the warning the current behavior is what we want.
Seems like it might be the less intrusive change for the moment
|
This approach requires changes to many patterns: you have to add I have something different in mind that I would like to try first: removing the Can you check if this improves the situation? llvm/llvm-project#129861 |
| @@ -688,8 +688,9 @@ class OpConversionPattern : public ConversionPattern { | |||
| return matchAndRewrite(op, OpAdaptor(oneToOneOperands, adaptor), rewriter); | |||
| } | |||
|
|
|||
| private: | |||
| protected: | |||
There was a problem hiding this comment.
The using here is only to silence warnings. I think it won't be a problem to turn it into protected.
I said more on Discourse, but the short answer is that your change does not fix these warnings and still requires adding |
|
Should we close this one? |
|
Abandoned in favor of #1440 |
There is a difference in behavior between clang and gcc that was leading to a very large number of -Woverloaded-virtual warnings when compiling with gcc.
The difference in behavior is that gcc warns for "partial" virtual overloads. That is, when a base class contains multiple virtual functions with the same name and a derived class overrides at least one but not all of them, gcc issues a warning saying that the non-overridden variations were hidden. This diagnostic is only reported if the -Woverloaded-virtual is explicitly specified on the command line, which it is by the clang project.
The standard way to suppress this warning, indicating that the behavior was intentional (which it was in all cases I'm fixing here) is to add a "using" statement for the overloaded symbol in the derived class. However, there was a secondary problem that this couldn't be done in the ClangIR classes because of a "using" clause in the base class that was marked as "private".
This change updates a few files in the MLIR, both to allow subclasses to add their own "using" statements by making the base class "using" statement protected, and also fixes a large number of places where the derived class "using" statement was needed in MLIR dialects, in addition to adding such statements where needed in the ClangIR code.
The MLIR changes will likely be made upstream, but I am adding them here in the clangir incubator as an proof-of-concept to show what will be needed in downstream projects.