Skip to content

Conversation

@stellaraccident
Copy link
Collaborator

  • Exposes the op registry via a get_registered_ops method.
  • Moves the aten dialect generation scripts in prep for integrating them with this facility.

Copy link
Contributor

@silvasean silvasean left a comment

Choose a reason for hiding this comment

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

Is there any meaningful test for this? Maybe it should wait for some later functionality to materialize. Feel free to skip.

record["returns"] = std::move(returns);
results.append(std::move(record));
});
{ dispatcher.addRegistrationListener(std::move(listener)); }
Copy link
Contributor

Choose a reason for hiding this comment

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

I find these braces hard to understand. Why are they needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ugh. Added this note:

  // Note: addRegistrationListener reports all currently registered ops
  // during the call and then incrementally reports newer ops until the RAII
  // return value is destroyed. Since we only want the current, surround in
  // a block so it immediately unregisters.

I didn't trace through any asynchronicity that may be involved and just wanted to ensure there was an unregistration prior to manipulating further.

@stellaraccident
Copy link
Collaborator Author

Is there any meaningful test for this? Maybe it should wait for some later functionality to materialize. Feel free to skip.

I started adding a trivial FileCheck test, then realized that the test tree needs refactoring, and we should have a docker container anyway. Since this is the first meaningful functionality in the "new" way, let me check it in as-is and I'll untangle the test situation next.

* Exposes the op registry via a get_registered_ops method.
* Moves the aten dialect generation scripts in prep for integrating them with this facility.
@stellaraccident
Copy link
Collaborator Author

FYI - added docker container and fixed the test tree in 0cb28f0 (and added a trivial test for this)

qedawkins pushed a commit to nod-ai/torch-mlir that referenced this pull request Oct 3, 2022
Co-authored-by: Gheorghe-Teodor Bercea <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants