Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interfaces #3

Open
wants to merge 35 commits into
base: tutorial_ch2
Choose a base branch
from
Open

Interfaces #3

wants to merge 35 commits into from

Conversation

sana-damani
Copy link
Collaborator

Inliner currently does not work:

doesn't handle generic calls
adding new pass results in build break
Also need feedback on how to test shape inference.

River707 and others added 30 commits September 10, 2019 12:50
This effectively rewrites Ch.2 to introduce dialects, operations, and registration instead of deferring to Ch.3. This allows for introducing the best practices up front(using ODS, registering operations, etc.), and limits the opaque API to the chapter document instead of the code.

PiperOrigin-RevId: 265614997
This effectively rewrites Ch.2 to introduce dialects, operations, and registration instead of deferring to Ch.3. This allows for introducing the best practices up front(using ODS, registering operations, etc.), and limits the opaque API to the chapter document instead of the code.

PiperOrigin-RevId: 265614997
class ShapeInferenceInterface
: public DialectInterfaceCollection<DialectShapeInferenceInterface> {
public:
using Base::Base;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be written as an OpInterface that is registered on each of the operations?

>];
}

def GenericCallOp : Toy_Op<"generic_call"> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To hook into inlining, we will need to add the CallOpInterface to this op. You can look at the Standard call op for an example:
https://github.com/tensorflow/mlir/blob/master/include/mlir/Analysis/CallInterfaces.td

/// as necessary.
void handleTerminator(Operation *op,
ArrayRef<Value *> valuesToRepl) const final {
// Only "toy.return" needs to be handled here.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably mentioned that "toy.return" is the only terminator in the toy dialect.

// Analysis Hooks
//===--------------------------------------------------------------------===//

bool shouldAnalyzeRecursively(Operation *op) const final { return true; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hook doesn't need to be override, this is only necessary on operations with regions.

void inferShape(Operation *op) {
// The add operation is trivial: propagate the input type as is.
if (auto addOp = llvm::dyn_cast<AddOp>(op)) {
op->getResult(0)->setType(op->getOperand(0)->getType());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is an OpInterface, each of these can be handled directly by the ops themselves.

@River707
Copy link
Collaborator

River707 commented Oct 5, 2019

Hey Sana, the inliner pass was just updated upstream to include support for calls from different dialects. This should enable the inlining for toy, with a few tweaks. One thing we will likely need at this point is another pass that deletes all of the non-main functions. We don't have proper modeling for static/internal functions to be able to remove them generically, so we will need to do that in toy.

As for the shape inference interface, could we just have this as an OpInterface that gets implemented by the toy operations? Is there a reason why we can't test the shape inference pass via FileCheck? I'm assuming that it should fire if we put it in the pass pipeline. You could just write a .mlir that only has main where all of the shapes are still dynamic and try to run it through toy-ch4.

@joker-eph joker-eph changed the base branch from master to tutorial_ch2 October 5, 2019 21:27
@sana-damani
Copy link
Collaborator Author

sana-damani commented Oct 12, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants