This is an archive of the discontinued LLVM Phabricator instance.

Defend early against operation created without a registered dialect
ClosedPublic

Authored by mehdi_amini on Jul 13 2021, 10:49 PM.

Details

Diff Detail

Event Timeline

mehdi_amini created this revision.Jul 13 2021, 10:49 PM
mehdi_amini requested review of this revision.Jul 13 2021, 10:49 PM
rriddle requested changes to this revision.Jul 13 2021, 11:15 PM
rriddle added inline comments.
mlir/include/mlir/IR/BuiltinOps.td
266

Accidentally change here?

mlir/lib/IR/Operation.cpp
180

Can we check this in the other place we check unregistered operations?

This revision now requires changes to proceed.Jul 13 2021, 11:15 PM
jpienaar added inline comments.Jul 13 2021, 11:26 PM
mlir/lib/IR/Operation.cpp
185

s/mlir-opt/MLIR opt tool used/ ? :) [no change needed, I was just thinking most folks will probably not be using mlir-opt vs their own one at this time]

mehdi_amini added inline comments.Jul 13 2021, 11:31 PM
mlir/include/mlir/IR/BuiltinOps.td
266

No: the parser creates this op to have Value for forward references.
We could change the parser to use block arguments instead as well

mlir/lib/IR/Operation.cpp
180

What do you mean?
There isn’t another ctor is there?

rriddle added inline comments.Jul 13 2021, 11:46 PM
mlir/include/mlir/IR/BuiltinOps.td
266

Ah, I see. Can we change the parser to use unrealized_conversion_cast or block arguments, as opposed to defining a new op in the builtin dialect?

mlir/lib/IR/Operation.cpp
180

Right, I was thinking of a check that I've discussed with someone before but it never got added. Can we move this check to OperationName instead?

mehdi_amini added inline comments.Jul 14 2021, 12:45 PM
mlir/include/mlir/IR/BuiltinOps.td
266

Yeah I'll do that.

mlir/lib/IR/Operation.cpp
180

We could, I'm slightly concerned that some folks may be creating an OperationName for the sake of trying to get an AbstractOperation.

  • Use unrealized_cast in the parser
  • Update error message
mehdi_amini marked an inline comment as done.Jul 14 2021, 2:53 PM
mehdi_amini added inline comments.
mlir/lib/IR/Operation.cpp
180

First hiccup with the OperationName approach: we create patterns this way!

Pattern::Pattern(StringRef rootName, PatternBenefit benefit,
                 MLIRContext *context, ArrayRef<StringRef> generatedNames)
    : Pattern(OperationName(rootName, context).getAsOpaquePointer(),
              RootKind::OperationName, generatedNames, benefit, context) {}

And we do it regardless of a dialect being loaded in the context, since we may create these on initialization and before the dialect is actually loaded.

185

Done! (and updated the other existing places with the same message while at it)

mehdi_amini marked 3 inline comments as done.Jul 14 2021, 2:54 PM
mehdi_amini added inline comments.
mlir/include/mlir/IR/BuiltinOps.td
266

I struggled to use BlockArguments in the parser, I ended up using unrealized_cast!

mlir/lib/IR/Operation.cpp
180

I struggled to use BlockArguments in the parser, I ended up using unrealized_cas

rriddle added inline comments.Jul 14 2021, 3:12 PM
mlir/lib/IR/Operation.cpp
180

That is part of the problem. The current way we have OperationName structured makes creating it before loading a dialect problematic: i.e. an OperationName created before is != to one created after loading.

rriddle accepted this revision.Jul 14 2021, 3:14 PM
rriddle added inline comments.
mlir/lib/IR/Operation.cpp
180

I'm fine with resolving that separately though. I think we should just switch to lazy loading like we do with dialect names in Identifiers. That would also lead to a performance improvement, by removing PointerUnion interaction.

This revision is now accepted and ready to land.Jul 14 2021, 3:14 PM
mehdi_amini added inline comments.Jul 14 2021, 6:01 PM
mlir/lib/IR/Operation.cpp
180

OperationName created before is != to one created after loading.

Wait, but how do things work right now? It is likely common that we create patterns for ops that aren't registered yet, isn't it?

rriddle added inline comments.Jul 14 2021, 6:11 PM
mlir/lib/IR/Operation.cpp
180

If a pattern has an OperationName from before the op was registered, it won't match a registered operation AFAIK.

mehdi_amini added inline comments.Jul 14 2021, 7:13 PM
mlir/lib/IR/Operation.cpp
180

Surprising, matching an identifier should be enough, shouldn't we just keep an identifier pointer?

This change introduced a failure on Windows, but only when compiling with Visual Studio which is why it did not impact the buildbots (or really, almost any other uses cases, since most people use ninja). It came up as part of onnx-mlir merge with the latest llvm (https://github.com/onnx/onnx-mlir/pull/814) and I can confirm that I see the same failure locally.

It looks like when we build with Visual Studio, regardless of the state of LLVM_ENABLE_ASSERTIONS, enable_assertions is set to 0 because of this piece of code in AddLLVM.cmake (https://github.com/llvm/llvm-project/blob/main/llvm/cmake/modules/AddLLVM.cmake):

if(LLVM_ENABLE_ASSERTIONS AND NOT MSVC_IDE)
  set(ENABLE_ASSERTIONS "1")
else()
  set(ENABLE_ASSERTIONS "0")
endif()

I think we have two options:

  1. change the new test to be able to pass regardless of the state of assertions or change the logic in AddLLVM.cmake
  2. change the logic in AddLLVM.cmake

(or, 3), I suppose, add a new config property which can track LLVM_ENABLE_ASSERTIONS regardless of MSVC_IDE, but I think that's a mistake).

I *think*, though I have not fully tested it yet, that changing the logic in AddLLVM.cmake will have no adverse effects on the builds/tests. It looks like this piece of code has been like that since 2011 and both the cmake setup for llvm and Visual Studio have changed sufficiently, that is is probably no longer necessary.

Thanks for tracking it down @stella.stamenova ; indeed the original commit is:

commit b46fdac4609df2613177813d43a124e3d9a8a306
Author: Andrew Trick <atrick@apple.com>
Date:   Tue Jun 28 16:32:01 2011

    cmake: Our MSVC build does not support config-time build mode.
    
    llvm-svn: 134008

There isn't much more info. If you remove the special case for MSVC_IDE and it works that'd be ideal.
Otherwise there is also the option to disable this test on windows for now.

Thanks for tracking it down @stella.stamenova ; indeed the original commit is:

commit b46fdac4609df2613177813d43a124e3d9a8a306
Author: Andrew Trick <atrick@apple.com>
Date:   Tue Jun 28 16:32:01 2011

    cmake: Our MSVC build does not support config-time build mode.
    
    llvm-svn: 134008

There isn't much more info. If you remove the special case for MSVC_IDE and it works that'd be ideal.
Otherwise there is also the option to disable this test on windows for now.

I've confirmed that we don't need the workaround for MSVC_IDE in the LLVM cmake file any more and it does not impact any of the other tests elsewhere in the project. @mehdi_amini : I can make the commit to AddLLVM.cmake or you could do it to fix the mlir test. Let me know.

The fix in AddLLVM.cmake seems best, I didn't quite get what we'd fix in the test itself?

The fix in AddLLVM.cmake seems best, I didn't quite get what we'd fix in the test itself?

I meant: I can make the change or you can, what would you prefer?

The fix in AddLLVM.cmake seems best, I didn't quite get what we'd fix in the test itself?

I meant: I can make the change or you can, what would you prefer?

Ah, please go ahead :)