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

[QDQ] Add shared qdq selectors #10178

Merged
merged 20 commits into from
Jan 12, 2022
Merged

[QDQ] Add shared qdq selectors #10178

merged 20 commits into from
Jan 12, 2022

Conversation

YUNQIUGUO
Copy link
Contributor

@YUNQIUGUO YUNQIUGUO commented Jan 4, 2022

Description: Describe your changes.

This is one of the first steps of NNAPI QDQ Integration: (step 3 in this pr description #10052)

  • Add a shared SelectorManager object which extracted selectors related logic from the original selectionactiontransformer.
  • Add sample test for usage demonstration.

@YUNQIUGUO YUNQIUGUO changed the title [QD] [QDQ] Add shared qdq selectors Jan 4, 2022

struct Selector {
using OpVersionsMap = std::unordered_map<std::string, std::vector<ONNX_NAMESPACE::OperatorSetVersion>>;

Copy link
Contributor

Choose a reason for hiding this comment

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

If we'll generally be looking up a specific operator version, an unordered_map may be a better fit than a vector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thought operatorsetversion would be a set of ints.. Can you clarify how would this unordered_map be used/how does the map key/value look like?

void RegisterUnarySelectors(Selectors& qdq_selectors) {
/* regsiter selectors for unary ops */
std::unique_ptr<BaseSelector> selector(new QDQ::UnarySelector());
qdq_selectors.RegisterSelector(Selector::OpVersionsMap{{"AveragePool", {}}},
Copy link
Contributor

@guoyu-wang guoyu-wang Jan 5, 2022

Choose a reason for hiding this comment

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

This line is already updated in here

qdq_selector_action_registry.RegisterSelectorAndAction(action_name,
{{"AveragePool", {}},
{"LeakyRelu", {}}},

To avoid the fork of the code, can we keep them in a single place for these keys?
For example, some shared function to return this OpVersionsMap (without alias)

static const std::unordered_map<std::string, std::vector<ONNX_NAMESPACE::OperatorSetVersion>> GetUnaryOpVersionsMap() {
    return {{"AveragePool", {}},{"LeakyRelu", {}}};
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Add some static methods for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

will qdq_selector_action_transformer.cc be updated to use the same helpers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the reminder. will keep them updated together later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Things are going to diverge so I don't think we should try and keep it in sync with the CPU EP's QDQ SAT.

The QDQ node group selectors for use by other EPs are looking for all QDQ node groups. The CPU EP is looking for things it has QLinear implementations for.

Additionally the CPU EP will be registering NodeSelector based classes in SelectorAndAction. The QDQ node group selectors will be registering NodeGroupSelector based classes.

So they will look somewhat similar initially but I expect as we keep expanding the ops we match as QDQ node groups that won't be for long.

e.g. SoftMax should be matched in NNAPI but not the CPU EP. Similarly we currently have only Add and Mul, however (afaik) there's no reason Sub and Div shouldn't be matched and those can easily be added to the NodeGroupSelector registrations.

#include "core/optimizer/qdq_transformer/selectors_actions/qdq_selector_action_transformer.h"
#include "core/optimizer/qdq_transformer/selectors_actions/qdq_selectors.h"

#include "utils.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

this one should be first

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, to clarify, I meant it should be the very first include. the rationale for this is to ensure that the related header can be included on its own and not require other external includes before it.

void Initialize();

std::vector<NodeGroup> GetQDQSelections(const GraphViewer& graph_viewer) const;

Copy link
Contributor

Choose a reason for hiding this comment

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

Some documentation for non-trivial public methods would be good.

e.g. Initialize is self-explanatory. GetQDQSelections could provide a quick overview of what happens. Imagine you're a new developer trying to understand what the class does. Without that info you're forced to read the entire implementation of the method to know this. That's both time consuming and potentially error prone if something in the implementation is misinterpreted.

Copy link
Contributor

@skottmckay skottmckay left a comment

Choose a reason for hiding this comment

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

:shipit:

const auto qdq_node_group_selection = op_versions_and_selector.selector->GetQDQSelection(graph_viewer, *node);
if (qdq_node_group_selection.has_value()) {
const auto& qdq_group = *qdq_node_group_selection;
qdq_selections.push_back(qdq_group);
Copy link
Contributor

@guoyu-wang guoyu-wang Jan 11, 2022

Choose a reason for hiding this comment

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

nit, can be more concise, can be updated in a later PR

if (qdq_node_group_selection) {
      qdq_selections.push_back(*qdq_node_group_selection);
}

@@ -1826,5 +1827,38 @@ TEST(QDQTransformerTests, QDQ_Selector_Test) {
ASSERT_FALSE(result.has_value());
}
}

TEST(QDQTransformerTests, QDQ_Shared_GetSelectors_Test) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, can try to combine this test case with the QDQ_Selector_Test test case
Can be updated in a later PR

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.

4 participants