This is an archive of the discontinued LLVM Phabricator instance.

[mlir][ods] ODS ops get an `extraClassDefinition`
ClosedPublic

Authored by Mogball on Dec 14 2021, 10:36 PM.

Details

Summary

Extra definitions are placed in the generated source file for each op class. The substitution $cppClass is replaced by the op's C++ class name.

This is useful when declaring but not defining methods in TableGen base classes:

class BaseOp<string mnemonic> 
    : Op<MyDialect, mnemonic, [DeclareOpInterfaceMethods<SomeInterface>] {
  let extraClassDeclaration = [{ 
    // ZOp is declared at at the bottom of the file and is incomplete here
    ZOp getParent();
  }];
  let extraClassDefinition = [{
    int $cppClass::someInterfaceMethod() {
      return someUtilityFunction(*this);
    }
    ZOp $cppClass::getParent() {
      return dyn_cast<ZOp>(this->getParentOp());
    }
  }];
}

Certain things may prevent defining these functions inline, in the declaration. In this example, ZOp in the same dialect is incomplete at the function declaration because ops classes are declared in alphabetical order. Alternatively, functions may be too big to be desired as inlined, or they may require dependencies that create cyclic includes, or they may be calling a templated utility function that one may not want to expose in a header. If the functions are not inlined, then inheriting from the base class N times means that each function will need to be defined N times. With extraClassDefinitions, they only need to be defined once.

Diff Detail

Event Timeline

Mogball created this revision.Dec 14 2021, 10:36 PM
Mogball requested review of this revision.Dec 14 2021, 10:36 PM
rriddle requested changes to this revision.EditedDec 14 2021, 10:50 PM

What types of use cases is this intended for? E.g. what examples do you have that would use this?

This revision now requires changes to proceed.Dec 14 2021, 10:50 PM

The use cases I have in mind are both related to declaring but not defining methods in TableGen base classes.

class BaseOp<string mnemonic> : Op<MyDialect, mnemonic, [DeclareInterfaceMethods<SomeInterface>] {
  let extraClassDeclaration = [{ 
    // ZOp is declared at at the bottom of the file and is incomplete here
    ZOp getParent();
  }];
}

If I inherit from this class N times, I have to define N identical getParent methods and interface methods, and usually I have a templated function hiding in the source file I don't want to put in the header.

Can you capture such motivation in the commit message?

Mogball edited the summary of this revision. (Show Details)Dec 15 2021, 1:29 PM
jpienaar added inline comments.
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
563

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp:2125, isn't there already the convention of using cppClass ?

Mogball added inline comments.Dec 16 2021, 5:50 AM
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
563

Oh, yeah. I didn't realize there wasn't an underscore.

Mogball updated this revision to Diff 395455.Dec 20 2021, 8:07 AM

Rename the substitution _cppClass -> cppClass and update description

Mogball edited the summary of this revision. (Show Details)Dec 20 2021, 8:11 AM
Mogball marked an inline comment as done.
rriddle accepted this revision.Jan 5 2022, 1:56 PM

Is there a place in OpDefinitions.md to fit this?

The justification for base class stuff makes sense to me. I think we should discourage this for most other situations though.

mlir/include/mlir/IR/OpBase.td
2449–2450

Might want to add clarification here about where the generated code is placed w.r.t. the ops namespace.

This revision is now accepted and ready to land.Jan 5 2022, 1:56 PM
Mogball updated this revision to Diff 397758.Jan 5 2022, 5:42 PM

Update docs

Mogball marked an inline comment as done.Jan 5 2022, 5:43 PM
This revision was landed with ongoing or failed builds.Jan 5 2022, 5:43 PM
This revision was automatically updated to reflect the committed changes.