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

[DRAFT] Support CircleGRU #12319

Conversation

BalyshevArtem
Copy link
Contributor

@BalyshevArtem BalyshevArtem commented Dec 18, 2023

This draft supports CircleGRU in compiler.

for issue: #12320
related issue: #12263

ONE-DCO-1.0-Signed-off-by: Artem Balyshev [email protected]

@BalyshevArtem BalyshevArtem added the DRAFT A draft issue or PR for sharing one's current working status and discussion. label Dec 18, 2023
@BalyshevArtem BalyshevArtem marked this pull request as draft December 18, 2023 12:39
@BalyshevArtem BalyshevArtem force-pushed the draft_compiler_support_circle_gru branch from 29060f0 to 6ee3e3e Compare January 15, 2024 15:25
@@ -58,6 +58,7 @@ class CircleOpRegistry
REG_TFL_OP(BATCH_MATMUL, CircleOpBatchMatMul);
REG_TFL_OP(BCQ_FULLY_CONNECTED, CircleOpBCQFullyConnected);
REG_TFL_OP(BCQ_GATHER, CircleOpBCQGather);
REG_TFL_OP(CIR_GRU, CircleOpCircleGRU);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
REG_TFL_OP(CIR_GRU, CircleOpCircleGRU);
REG_TFL_OP(GRU, CircleOpGRU);

Copy link
Contributor

Choose a reason for hiding this comment

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

or,
1/ to prepare tflite introduces GRU,
2/ two Circle looks odd

Suggested change
REG_TFL_OP(CIR_GRU, CircleOpCircleGRU);
REG_TFL_OP(CIR_GRU, CircleOpCirGRU);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think second variant better to avoid future problems with tflite GRU operation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -64,6 +64,7 @@ enum Activation {
NONE = 0;
RELU = 1;
RELU6 = 3;
TANH = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

Q) is this necessary for our model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove it, thank you

@@ -541,6 +541,13 @@ class BuiltinOptionsExtractor final
to_circle_actfunc(node->fusedActivationFunction()))
.Union();
}
flatbuffers::Offset<void> visit(luci::CircleGRU *node)
Copy link
Contributor

Choose a reason for hiding this comment

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

CircleGRU -> need to sync name with chef.
I don't think there is one exact answer so we may need to discuss.

Comment on lines 239 to 241
if (num_fc != 2 or num_mul != 3 or num_logistic != 2 or num_split != 2 or num_add != 6 or
num_gather != 1 or num_reshape != 1 or num_sub != 1 or num_tanh != 1 or num_split_out != 6)
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think counting elements can provide correct condition.
And tracking the graph for exact Op type looks laborious job.
But we may have to do with some good graph traversal codes.

Copy link
Contributor Author

@BalyshevArtem BalyshevArtem Jan 16, 2024

Choose a reason for hiding this comment

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

Okay, I will change it further )

@seanshpark
Copy link
Contributor

seanshpark commented Jan 15, 2024

May need res/TensorFlowLiteRecipes/Net_DecomposedGru_000 for testings

  • circle2circle-dredd-recipe-test
  • luci-pass-value-py-test (need luci-interpreter)

You may know that you can create Net_DecomposedGru_000 from tflite file with tflchef-reverse tool.

@seanshpark
Copy link
Contributor

Q) do you not have a plan to implement luci-interpreter ?

cloned->rank(node->rank());

// values
switch (node->dtype())
Copy link
Contributor

Choose a reason for hiding this comment

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

Q) do we need all these implemented types?

if (not while_out_node)
continue;

if (fuse_gru(while_out_node))
Copy link
Contributor

Choose a reason for hiding this comment

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

Q) are other sub graphs like the body graph removed?
I don't recall we had some method to remove graph in the Module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Q) are other sub graphs like the body graph removed?

Actually - no. We need to support some methods to remove subgraphs in the Module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about add a new option that will leave only the main sub-graph, and delete others? Smth like --use_only_main_graph and use fuse_gru and use_only_main_graph together. Otherwise, we need to transfer/save the luci::Module entity to CircleOptimizer to remove unnecessary graphs

Copy link
Contributor

@seanshpark seanshpark Feb 2, 2024

Choose a reason for hiding this comment

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

Otherwise, we need to transfer/save the luci::Module entity to CircleOptimizer to remove unnecessary graphs

Maybe some kind of eliminate dead sub-graph...
1/ As of now, all sub-graphs having ID > 0 are used by some other sub-graph (usually ID == 0).
(I'm not sure this will hold in the future)
2/ We can use void CircleOptimizer::optimize(luci::Module *m) const
3/ Add something like EliminateDeadSubgraphPass (maybe with --elminate-dead-graph option or run this for default without any options)
4/ Traverse main graph and mark sub-graphs that are used
5/ Traverse used sub-graphs and mark other used sub-graphs too
6/ Delete sub-graphs that are not marked

Can this be feasible ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can this be feasible ?

I think yes, good idea to add EliminateDeadSubgraphPass and run this for default

@BalyshevArtem
Copy link
Contributor Author

Q) do you not have a plan to implement luci-interpreter ?

I'm not sure if this operation is needed in luci-interpreter, so now I don't have this in plan. But if you think it's better to add it, I'll do it)

@seanshpark
Copy link
Contributor

But if you think it's better to add it, I'll do it)

Current purpose I have is to validate fusion implementation through value test (as mathematical equivalence).
If adding the kernel takes heavy work, we may fire a task issue and postpone to later time.
I'll let you decide by your situation.

@BalyshevArtem
Copy link
Contributor Author

You may know that you can create Net_DecomposedGru_000 from tflite file with tflchef-reverse tool.

I got assertion tflchef-reverse: /ONE/compiler/tflchef/tflite/src/RecipeChef.cpp:77: std::unique_ptrtflchef::ModelRecipe tflchef::generate_recipe(const tflite::Model*): Assertion `tflite_import.num_subgraph() == 1' failed.

@seanshpark
Copy link
Contributor

`tflite_import.num_subgraph() == 1' failed.

current tflchef-reverse seems to supports only single subgraph model.
you can expand this to support multiple subgraphs or post an issue to request this feature.

@BalyshevArtem BalyshevArtem force-pushed the draft_compiler_support_circle_gru branch from 4ed032d to aac9083 Compare February 1, 2024 08:07
@BalyshevArtem
Copy link
Contributor Author

May need res/TensorFlowLiteRecipes/Net_DecomposedGru_000 for testings

Done, added it

@BalyshevArtem
Copy link
Contributor Author

BalyshevArtem commented Feb 2, 2024

@seanshpark, can I split this draft into PRs? For luci/pass/src/FuseGRUPass pass I think better create another draft to solve this problem #12319 (comment) and this #12319 (comment) and add luci-interpreter part to check values.

@seanshpark
Copy link
Contributor

seanshpark commented Feb 2, 2024

can I split this draft into PRs?

Yes, let's go :)

I didn't look deeper into the draft codes... I check while review with PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DRAFT A draft issue or PR for sharing one's current working status and discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants