Adds option to specify graph entry point name #1663

kernhanda · 2022-09-01T05:38:02Z

This changes the behavior when a single entry point is specified. Prior to this change, the entry point would be ignored if there was only a single entry point and run_main_graph would be emitted. After this change, the entry point name is honored, even if there's only one entry point.

jenkins-droid · 2022-09-01T05:52:15Z

Can one of the admins verify this patch?

jenkins-droid · 2022-09-01T05:52:18Z

Can one of the admins verify this patch?

tungld · 2022-09-01T11:56:29Z

@jenkins-droid test this please

tungld · 2022-09-06T00:41:32Z

@kernhanda thank you for the patch! could you please look at why lit tests failed? You can check it locally by using make check-onnx-lit. Thanks!

Signed-off-by: Kern Handa <kern.handa@gmail.com>

sstamenova · 2022-09-06T20:17:13Z

test/mlir/krnl/entry_point.mlir

@@ -13,7 +11,7 @@ module {
 // CHECK:         llvm.mlir.global external constant @_entry_point_0_in_sig("[in_sig]\00")
 // CHECK:         llvm.mlir.global external constant @_entry_point_0_out_sig("[out_sig]\00")

-// CHECK-LABEL:   llvm.func @run_main_graph
+// CHECK-LABEL:   llvm.func @run_first_entry


Shouldn't the default continue to be generated as run_main_graph?

Also, we should have at least one test to validate providing a different name.

Shouldn't the default continue to be generated as run_main_graph?

This is open for debate, IMO. I'm fine if the answer is "yes", but I'd like some clarification.

With this change, if the user provides their own graph name, thus overriding main_graph, for an ONNX model, the entry point emitted would still be run_main_graph, somewhat defeating the purpose. I could update the behavior such that run_main_graph is emitted if the user didn't override the default graph name.

The other scenario is where an existing MLIR file is being compiled, where there is only one kernel entry point specified. In this case, the current behavior is to disregard the name of the entry point and always emit run_main_graph. TBH, this doesn't make sense to me, but if there are reasons/dependencies that rely upon this behavior, I can make the change more selective.

Also, we should have at least one test to validate providing a different name.

I'll add an ONNX model graph test, then.

So in the source, main_graph is still the default sometimes. There should be a test for this and if it's never the default, it should not be in the source. It sounds like we need at least 3 tests for the entry points.

Updated the test to ensure the default scenario is tested and then a scenario where the single entry point has a non-default name.

So the three tests I would expect to see are:

default "main_graph". My understanding is that if no name is specified, this is the name picked. Now both tests are specifying a name (though one of them is specifying "main_graph")

entry point with name in the graph

Using the command line option to specify a name other than "main_graph"

Are there any existing tests that import a basic ONNX model? I see that the repository has a mnist.onnx in the docs directory, but AFAICT, there are no tests that act on an ONNX model.

We don't have tests that import an ONNX model directly but @sorenlassen recenlty add tests that create a small ONNX model in JSON and pass it to onnx-mlir command line. One example is: https://github.com/onnx/onnx-mlir/blob/main/test/mlir/onnx/parse/test_abs.json. Hope it helps.

jenkins-droid · 2022-09-06T20:35:19Z

Can one of the admins verify this patch?

jenkins-droid · 2022-09-06T20:35:21Z

Can one of the admins verify this patch?

Signed-off-by: Kern Handa <kern.handa@gmail.com>

jenkins-droid · 2022-09-06T22:06:16Z

Can one of the admins verify this patch?

jenkins-droid · 2022-09-06T22:11:10Z

Can one of the admins verify this patch?

kernhanda · 2022-09-06T23:25:41Z

@jenkins-droid test this please

jenkins-droid · 2022-09-06T23:25:51Z

Can one of the admins verify this patch?

jenkins-droid · 2022-09-06T23:26:51Z

Can one of the admins verify this patch?

tungld · 2022-09-07T01:19:53Z

@jenkins-droid test this please

tungld · 2022-09-07T01:45:39Z

I see a big advantage of specifying a custom entry point name: users have two (single entry) onnx models and they want to run them in the same Python/C program. In this case, entry points should be different.

I could update the behavior such that run_main_graph is emitted if the user didn't override the default graph name.

I prefer this. Since we say that default graph name is main_graph when importing an onnx model, it's clear to users. Also, Java and C interfaces are using run_main_graph as the default entry point.

The other scenario is where an existing MLIR file is being compiled, where there is only one kernel entry point specified. In this case, the current behavior is to disregard the name of the entry point and always emit run_main_graph. TBH, this doesn't make sense to me, but if there are reasons/dependencies that rely upon this behavior, I can make the change more selective.

I am not so convince that onnx-mlir users will compile an MLIR file as input. It's mainly for onnx-mlir developers. So it looks fine to use the entry point as it is.

kernhanda · 2022-09-19T20:23:48Z

I see a big advantage of specifying a custom entry point name: users have two (single entry) onnx models and they want to run them in the same Python/C program. In this case, entry points should be different.

Yes, that's exactly the motivation here.

I prefer this. Since we say that default graph name is main_graph when importing an onnx model, it's clear to users. Also, Java and C interfaces are using run_main_graph as the default entry point.

With this change, if you import a model and don't specify the graph name, it does default to main_graph.

I am not so convince that onnx-mlir users will compile an MLIR file as input. It's mainly for onnx-mlir developers. So it looks fine to use the entry point as it is.

Do you think it's necessary to make the change so that if a MLIR file has a single entry point named something other than main_graph that it should still result in an entry point named run_main_graph?

^ This is the only question at the moment as it's the only difference in existing functionality. Otherwise, if you don't specify the graph name option, the behavior is the same as it is without this change.

tungld · 2022-09-20T04:52:36Z

Do you think it's necessary to make the change so that if a MLIR file has a single entry point named something other than main_graph that it should still result in an entry point named run_main_graph?

If users specify an entry point in an MLIR via onnx.EntryPoint we should respect that and should not change to main_graph. In summary we only allow users to set an entry point (main_graph by default) when importing an onnx model that does not have a concept of entry point. What do you think?

kernhanda · 2022-09-20T21:04:34Z

Do you think it's necessary to make the change so that if a MLIR file has a single entry point named something other than main_graph that it should still result in an entry point named run_main_graph?

If users specify an entry point in an MLIR via onnx.EntryPoint we should respect that and should not change to main_graph. In summary we only allow users to set an entry point (main_graph by default) when importing an onnx model that does not have a concept of entry point. What do you think?

I agree! This PR matches that functionality then 😄

@sstamenova requested tests that exercise the model import scenario, but I couldn't find any existing tests that do this. Any guidance on how to do this or if it's necessary would be appreciated.

tungld · 2022-09-21T01:34:08Z

src/Conversion/KrnlToLLVM/KrnlEntryPoint.cpp

    std::string dynEntryPointName = "run_" + staticEntryPointFuncName.str();
-    if (singleEntryPoint)
-      dynEntryPointName = DEFAULT_DYN_ENTRY_POINT;


@kernhanda because we don't use singleEntryPoint anymore, could you please remove it elsewhere, e.g. in function arguments? Thanks!

tungld · 2022-11-01T02:27:48Z

@kernhanda any update on this?

tungld · 2023-06-29T05:07:39Z

@kernhanda we would like to have this functionality, so do you have time to update this PR? If not I can take it over and create a new PR. thanks!

jenkins-droid · 2024-07-16T19:57:25Z

Can one of the admins verify this patch?

kernhanda added 3 commits September 6, 2022 13:09

Adds option to specify graph entry point name

68b4863

Signed-off-by: Kern Handa <kern.handa@gmail.com>

clang-format

aba203e

Signed-off-by: Kern Handa <kern.handa@gmail.com>

Update entry_point.mlir test file

4656d9e

Signed-off-by: Kern Handa <kern.handa@gmail.com>

kernhanda force-pushed the specify_graph_name branch from 9f7cbef to 4656d9e Compare September 6, 2022 20:10

sstamenova suggested changes Sep 6, 2022

View reviewed changes

kernhanda added 2 commits September 6, 2022 13:57

Update entry_point.mlir

62439e9

Signed-off-by: Kern Handa <kern.handa@gmail.com>

Updated test

e72827a

Signed-off-by: Kern Handa <kern.handa@gmail.com>

Merge remote-tracking branch 'origin/main' into specify_graph_name

8cc2da6

Merge branch 'main' into specify_graph_name

edcf95e

tungld mentioned this pull request Sep 15, 2022

Allows the user to specify a module ID to emit unique functions #1694

Closed

tungld reviewed Sep 21, 2022

View reviewed changes

tungld mentioned this pull request Jun 28, 2023

Running two models in the same program #2342

Closed

kernhanda closed this Jul 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds option to specify graph entry point name #1663

Adds option to specify graph entry point name #1663

kernhanda commented Sep 1, 2022 •

edited

Loading

jenkins-droid commented Sep 1, 2022

jenkins-droid commented Sep 1, 2022

tungld commented Sep 1, 2022

tungld commented Sep 6, 2022

sstamenova Sep 6, 2022

kernhanda Sep 6, 2022 •

edited

Loading

sstamenova Sep 6, 2022

kernhanda Sep 6, 2022

sstamenova Sep 7, 2022

kernhanda Sep 19, 2022

tungld Sep 21, 2022

jenkins-droid commented Sep 6, 2022

jenkins-droid commented Sep 6, 2022

jenkins-droid commented Sep 6, 2022

jenkins-droid commented Sep 6, 2022

kernhanda commented Sep 6, 2022

jenkins-droid commented Sep 6, 2022

jenkins-droid commented Sep 6, 2022

tungld commented Sep 7, 2022

tungld commented Sep 7, 2022

kernhanda commented Sep 19, 2022

tungld commented Sep 20, 2022 •

edited

Loading

kernhanda commented Sep 20, 2022

tungld Sep 21, 2022

tungld commented Nov 1, 2022

tungld commented Jun 29, 2023

jenkins-droid commented Jul 16, 2024

Adds option to specify graph entry point name #1663

Adds option to specify graph entry point name #1663

Conversation

kernhanda commented Sep 1, 2022 • edited Loading

jenkins-droid commented Sep 1, 2022

jenkins-droid commented Sep 1, 2022

tungld commented Sep 1, 2022

tungld commented Sep 6, 2022

sstamenova Sep 6, 2022

Choose a reason for hiding this comment

kernhanda Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

sstamenova Sep 6, 2022

Choose a reason for hiding this comment

kernhanda Sep 6, 2022

Choose a reason for hiding this comment

sstamenova Sep 7, 2022

Choose a reason for hiding this comment

kernhanda Sep 19, 2022

Choose a reason for hiding this comment

tungld Sep 21, 2022

Choose a reason for hiding this comment

jenkins-droid commented Sep 6, 2022

jenkins-droid commented Sep 6, 2022

jenkins-droid commented Sep 6, 2022

jenkins-droid commented Sep 6, 2022

kernhanda commented Sep 6, 2022

jenkins-droid commented Sep 6, 2022

jenkins-droid commented Sep 6, 2022

tungld commented Sep 7, 2022

tungld commented Sep 7, 2022

kernhanda commented Sep 19, 2022

tungld commented Sep 20, 2022 • edited Loading

kernhanda commented Sep 20, 2022

tungld Sep 21, 2022

Choose a reason for hiding this comment

tungld commented Nov 1, 2022

tungld commented Jun 29, 2023

jenkins-droid commented Jul 16, 2024

kernhanda commented Sep 1, 2022 •

edited

Loading

kernhanda Sep 6, 2022 •

edited

Loading

tungld commented Sep 20, 2022 •

edited

Loading