matchAndRewrite hiding virtual functions

MLIR
1

I have a question about compiler warnings that are being generated when I build LLVM with ClangIR enabled. I don’t think the warnings I’m asking about are specific to the ClangIR code, but they may not have been seen elsewhere because they only appear if you compile using GCC and explicitly specify the -Woverloaded-virtual option, which clang does.

The summary is that when -Woverloaded-virtual is used, GCC issues warnings about virtual functions that are hidden by declaring a function of the same name with a different signature in a derived class. Clang doesn’t give a warning about this. Here’s a small example showing the difference between GCC and clang behavior without bringing in MLIR:

https://godbolt.org/z/46edeKM5T

Now for the MLIR-specific part of this…

In ClangIR, we have a class CIRToLLVMGlobalOpLowering which derives from mlir::OpConversionPattern<cir::GlobalOp> and defines a method with this signature:

 mlir::LogicalResult
 matchAndRewrite(cir::GlobalOp op, OpAdaptor adaptor,
                 mlir::ConversionPatternRewriter &rewriter) const override;

When I compile this with GCC, I get a number of warnings similar to the following:

warning: 'llvm::LogicalResult mlir::OpConversionPattern<SourceOp>::matchAndRewrite(mlir::Operation*,
 llvm::ArrayRef<mlir::Value>, mlir::ConversionPatternRewriter&) const [with SourceOp = cir::GlobalOp]'
 was hidden [-Woverloaded-virtual]
  647 |   matchAndRewrite(Operation *op, ArrayRef<Value> operands,
      |   ^~~~~~~~~~~~~~~

It took me a while to understand exactly what’s happening. GCC is issuing a warning because mlir::OpConversionPattern<cir::GlobalOp> defines four virtual functions named matchAndRewrite (two of which are deprecated and marked final) and we’re only overriding one of them.

@erichkeane suggested adding a using mlir::OpConversionPattern<cir::GlobalOp>::matchAndRewrite; statement, but this produces an error because the mlir::OpConversionPattern<cir::GlobalOp> has a similar using statement for its base class, ConversionPattern, and that statement is declared private.

So, basically, I don’t think I can change the ClangIR code in a way that avoids this warning other than by doing something to suppress the warning. That is, I can’t fix the problem it’s reporting, only silence it. The “fix” – I think – would be to modify the MLIR base classes to use some kind impl pattern so the functions I’m overloading wouldn’t have a name conflict with functions that the class I’m deriving from is overloading, but that feels like it could have an unacceptable impact on downstream code.

To be clear, I don’t think there’s currently a real problem related to what GCC is warning about. I don’t expect anyone to intend to call the “hidden” functions. My only goal here is to get to a warning-free state.

There’s some discussion about GCC’s behavior here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=20423

There’s some discussion about removing the -Woverloaded-virtual from clang builds (and why they decided not to) here: :gear: D82617 Disable GCC’s -Woverloaded-virtual, which has false positives.

Has anyone else seen this? Any suggestions for the best way to handle it?

1 Like
2

It’s these 4 functions, right?

  • LogicalResult matchAndRewrite(Operation *op, ArrayRef<Value> operands, ConversionPatternRewriter &rewriter) const final
  • LogicalResult matchAndRewrite(Operation *op, ArrayRef<ValueRange> operands, ConversionPatternRewriter &rewriter) const final
  • virtual LogicalResult matchAndRewrite(SourceOp op, OpAdaptor adaptor, ConversionPatternRewriter &rewriter) const
  • virtual LogicalResult matchAndRewrite(SourceOp op, OneToNOpAdaptor adaptor, ConversionPatternRewriter &rewriter) const

two of which are deprecated and marked final

They are not deprecated, but you cannot override them.

Do you see these warning with any other patterns in MLIR? It is very common to derive from OpConversionPattern but only override the third one of the above functions. If this is triggering a warning, you should be seeing it all over the place in MLIR.

3

Yes, this is all over the place when compiling llvm/mlir with gcc.

4

Can you try if adding the using mlir::OpConversionPattern<cir::GlobalOp>::matchAndRewrite; after a private: works? I think this is not a good fix, but I’m wondering if it would make a difference. The fact that a function is hidden here is not a problem. (We’re not trying to call it through the derived class.)

I found this comment in the Clang code base:

        // If the method we are checking overrides a method from its base
        // don't warn about the other overloaded methods. Clang deviates from
        // GCC by only diagnosing overloads of inherited virtual functions that
        // do not override any other virtual functions in the base. GCC's
        // -Woverloaded-virtual diagnoses any derived function hiding a virtual
        // function from a base class. These cases may be better served by a
        // warning (not specific to virtual functions) on call sites when the
        // call would select a different function from the base class, were it
        // visible.
        // See FIXME in test/SemaCXX/warn-overload-virtual.cpp for an example.
5

The warning that I see all over the place is actually about rewrite, not matchAndRewrite:

llvm-project/mlir/include/mlir/IR/PatternMatch.h:255:16: warning: 'virtual void mlir::RewritePattern::rewrite(mlir::Operation*, mlir::PatternRewriter&) const' was hidden [-Woverloaded-virtual=]
  255 |   virtual void rewrite(Operation *op, PatternRewriter &rewriter) const;
      |                ^~~~~~~
llvm-project/mlir/include/mlir/Conversion/LLVMCommon/Pattern.h:192:16: note:   by 'mlir::ConvertOpToLLVMPattern<mlir::mpi::InitOp>::rewrite'
  192 |   virtual void rewrite(SourceOp op, OneToNOpAdaptor adaptor,

which I believe is is a valid warning, not a false positive.

I also get tons of warnings about dangling references and unused parameters.

6

Can you try adding another using directive for rewrite in the same file, line 214 next to this?

private:
  using ConvertToLLVMPattern::match;
  using ConvertToLLVMPattern::matchAndRewrite;

Looks like we forgot to add rewrite there.

1 Like
7

At least in one case I don’t see where I could add another private using. OpConversionPattern<...> derives from ConversionPattern which already uses matchAndRewrite privately (from RewritePattern). Hence adding private using rewrite yields an error in the instantiation.

When I make all the using XXX::rewrite protected the warnings about hidden rewrite do go away.

Would that be an acceptable solution?

8

Yes, I’ve tried that. I get this:

In file included from CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp:13:
CIR/Lowering/DirectToLLVM/LowerToLLVM.h:39:51: error: ‘virtual llvm::LogicalResult mlir::ConversionPattern::matchAndRewrite(mlir::Operation*, mlir::PatternRewriter&) const’ is private within this context
   39 |   using mlir::OpConversionPattern<cir::GlobalOp>::matchAndRewrite;
      |                                                   ^~~~~~~~~~~~~~~

The only thing I’ve found to get rid of the warning, other than not using -Woverloaded-virtual is to put this above the include statements:

#pragma GCC diagnostic ignored "-Woverloaded-virtual"

9

I’m seeing that one too.

10

I guess there’s a separate question regarding the clang behavior. There’s definitely some validity to what GCC is warning about, as seen in the Compiler Explorer I linked in my original post. If the signatures are different enough that a call to the hidden function can’t be coerced into a call to the exposed function, the compiler will give and error, but if, as in my example, the call can be coerced, the compiler will silently do the wrong thing.

I don’t think that’s a problem in the MLIR case where I’m seeing the warning. The hiding should be fine there.

11

I guess I jumped to a bad conclusion here. My statement about them being deprecated was based on this message but I didn’t look any further into what happened after that.

12

Shall I open a PR?

13

I’m trying to confirm that making these protected and then adding using...matchAndRewrite in the derived class fixes the warnings in ClangIR. It looks like some tblgen code will also need to be updated, but otherwise this looks like a promising direction to me.

14

I’ve posted a PR in the clangir incubator that fixes all places where this warning was being reported when I build the LLVM project with ClangIR enabled using gcc.

[CIR][MLIR] Fix overloaded virtual warnings by andykaylor · Pull Request #1439 · llvm/clangir

This PR includes changes to MLIR files, which I think include the changes @fschlimb was suggesting. Most of the ClangIR code I’m modifying in the PR hasn’t been upstreamed yet, but if an upstream change is made to bring in the MLIR changes, we will include the corresponding ClangIR changes in future upstreaming work for that project.

1 Like
15

Given that the vast majority of patterns use a combined matchAndRewrite instead of split match and rewrite, I think this part of the MLIR code base may be due for a cleanup. Can you check if this PR improves the situation? [mlir][IR] Move `match` and `rewrite` functions into separate class by matthias-springer · Pull Request #129861 · llvm/llvm-project · GitHub

16

it does!

1 Like
17

That may be a reasonable cleanup, but it doesn’t fix the problem I’m seeing with matchAndRewrite variations being hidden. You should be able to see this if you build the LLVM project with -DCLANG_ENABLE_CIR=ON using gcc, and it isn’t just limited to the CIR-specific files. Here’s an example where it’s reported in MLIR headers (with your patch applied) that are included from a CIR file:

In file included from llvm-project/llvm/../mlir/include/mlir/Interfaces/MemorySlotInterfaces.h:14,
                 from llvm-project/llvm/../mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h:19,
                 from llvm-project/llvm/../mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h:17,
                 from llvm-project/llvm/../mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h:18,
                 from llvm-project/clang/tools/cir-opt/cir-opt.cpp:17:
llvm-project/llvm/../mlir/include/mlir/IR/PatternMatch.h:336:17: warning: ‘llvm::LogicalResult mlir::detail::OpOrInterfaceRewritePatternBase<SourceOp>::matchAndRewrite(mlir::Operation*, mlir::PatternRewriter
&) const [with SourceOp = mlir::linalg::LinalgOp]’ was hidden [-Woverloaded-virtual]
  336 |   LogicalResult matchAndRewrite(Operation *op,
      |                 ^~~~~~~~~~~~~~~
In file included from llvm-project/llvm/../mlir/include/mlir/Dialect/Linalg/Passes.h:16,
                 from llvm-project/llvm/../mlir/include/mlir/InitAllPasses.h:31,
                 from llvm-project/clang/tools/cir-opt/cir-opt.cpp:19:
llvm-project/llvm/../mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h:1658: note:   by ‘virtual llvm::LogicalResult mlir::linalg::LinalgCopyVTWForwardingPattern::matchAndRewrite(mlir::vector::TransferWriteOp, mlir::PatternRewriter&) const’
 1658 |   LogicalResult matchAndRewrite(vector::TransferWriteOp xferOp,
      |

The reason it’s only showing up in this header when ClangIR is built is that the clang subproject adds -Woverloaded-virtual to the compile command-line and other subprojects don’t. You should see this if you compile the MLIR project by itself with -Woverloaded-virtual using gcc.

To fix it, I still need to add the using [base class]::matchAndRewrite; statement in every class that derives from OpConversionPattern and only defines one form of matchAndRewrite.

18

That’s what I suspected… I would’ve been surprised if all the warnings were gone. The ones about match() and rewrite() should be gone though.

We’re probably not going to be adding these to all patterns in MLIR. But your change to mlir/include/mlir/Transforms/DialectConversion.h looks reasonable to me.

Imo, instead of modifying all patterns, I would deactivate the -Woverloaded-virtual in the Clang IR build, but that’s your call to make.

2 Likes
19

It only needs to be deactivated for gcc, we can leave it on for clang and keep the codebase protected without the false positives (we already do this for other gcc warnings).

20

That seems like a reasonable approach. I’m preparing a patch to do that in the ClangIR code.