Loop operator #853

scxiao · 2021-06-09T15:35:44Z

This PR is for the implementation of the Loop operator for opset version 13.
Notes: 1) Default max iteration number is 10 if no max iteration number is provided
2) To change the max iter number, a user can set the max_loop_iterations in the onnx_option struct when parsing a model.
3) The returned shape of the scan output is from the max_loop_iterations even the actual loop num is less than that. This issue also applies to other operators like NonZero and NonMaxSuppression. A issue #948 is created to track this and to be resolved later.

…phX into inline_subgraph_try_removing_subgraph

… into inline_subgraph

…MIGraphX into inline_subgraph

… into loop_operator

…MIGraphX into inline_subgraph

… into loop_operator

… into inline_subgraph

… into loop_operator

pfultz2 · 2021-09-10T00:18:27Z

src/include/migraphx/op/loop.hpp

+
+        void append(const std::vector<argument>& iter_state,
+                    const std::vector<argument>& concatenated_outputs,
+                    const int iter) const


Dont make a value parameter const.

pfultz2 · 2021-09-10T00:28:57Z

src/include/migraphx/run_loop.hpp

+                      module_ref&, const std::unordered_map<std::string, argument>&)>& run)
+{
+    auto get_output_index = [](const std::string& name) {
+        std::string out_prefix = "#output_";


The naming of output indices as #output_ is specific to the GPU backend. Other backends could name them differently. Why do you need to check for the output params? Maybe this should added to the loop_model, maybe a function like std::map<std::string, int> get_output_params(const module& m)?

moved this to the gpu implementation

pfultz2 · 2021-09-10T00:32:14Z

src/include/migraphx/run_loop.hpp

+        if(loc != std::string::npos)
+        {
+            int index = std::stoi(name.substr(loc + out_prefix.size()));
+            return index;


It says there is no coverage here. There are other places where there is no coverage in this file we should probably add unit tests for. I assume there is no coverage because output params are only used by the GPU backend. Perhaps, we can create mock loop_model and some fake output params to check with the unit tests.

Yes, this part of code only applied to the GPU implementation.

pfultz2 · 2021-09-10T00:45:32Z

src/onnx/onnx_parser.cpp

+            pmod_insts_bkup[ins_p.first] = instructions[ins_p.first];
+        }
+        instructions[ins_p.first] = ins_p.second;
+    }


What is this doing? Is this to handle parameter shadowing? I thought shadowing is not permitted by ONNX spec from Nodes:

The graph MUST use single static assignment for all node outputs, which means that all node output names MUST be unique within a graph. In the case of a nested subgraph, a node output name MUST be distinct from the names from the outer scopes that are visible in the nested subgraph.

Is that an incorrect interpretation? Do you see onnx models that are shadowing params and constants?

We have another scenario that subgraph and parent graph have parameters with the same name. In this case, when we parse the subgraph, the instruction (stored in the mem var instructions corresponding to the parameter in the subgraph will overwrite that of the parent graph. After parsing subgraph returns, if we do not restore the parameter of the parent graph and and the parent graph uses this parameter as input for some other operators, inputs of the operator are the parameter of the subgraph.

In this scenario, the name is not output of any node, so the above specification does not apply, and there is no specification saying parent graph and subgraph cannot use the same name as parameter names. The only thing I read from the notes is:

In nested subgraphs used as attribute values, users MUST NOT use the same name as both a subgraph initializer and subgraph input unless the corresponding op's specification explicitly allows it.

but this does not apply, either.

I got the example with the above scenario some time ago online, but I cannot find it anymore. Also, when I created the onnx file from the script, no error is reported.

@pfultz2 Changed accordingly and added a unit test for the run loop. Could you please take a look again? Thanks

pfultz2 · 2021-09-10T00:46:34Z

src/targets/gpu/device/fill.cpp

+namespace gpu {
+namespace device {
+
+void fill(hipStream_t stream, const argument& result, const unsigned long& val)


Pass val by value.

pfultz2 · 2021-09-10T00:49:43Z

src/targets/gpu/lowering.cpp

+            auto cpu_cond =
+                mod->insert_instruction(ins, make_op("hip::copy_from_gpu"), inputs.at(1));
+            auto synced_max_iter =
+                mod->insert_instruction(ins, make_op("hip::sync_stream"), cpu_max_iter);


Shouldn't this take both cpu_max_iter and cpu_cond as inputs? So it will sync the stream after copying both variables?

pfultz2 · 2021-09-10T00:52:00Z

test/ref_ops_test.cpp

@@ -2439,6 +2439,100 @@ TEST_CASE(logsoftmax_test_axis_3)
    EXPECT(migraphx::verify_range(results_vector, s));
 }

+TEST_CASE(loop_test)


Can we put the ref loop unit tests in a seperate cpp file?

pfultz2 · 2021-09-10T00:53:59Z

test/ref_ops_test.cpp

+        }
+
+        return res;
+    };


I think it would be better to make this a regular function and make all the test cases seperate TEST_CASE cases in the test suite. This will make it easier to debug in the future as we can just run one test case(instead of all of them).

pfultz2 · 2021-09-10T23:34:29Z

src/onnx/onnx_parser.cpp

+            if(contains(instructions, name))
+            {
+                MIGRAPHX_THROW("module \"" + mod->name() + "\" has parameter name \"" + name +
+                               "\" existing in paraent graph!");


Sorry, fixed. Thanks!

pfultz2 · 2021-09-10T23:38:37Z

test/run_loop_test.cpp

+        return migraphx::shape(ins_out_shapes);
+    }
+
+    struct test_loop


Couldn't this inherit from ref_loop then just override get_output_params?

…GraphX into loop_operator

pfultz2 and others added 30 commits May 6, 2021 12:33

Fix tidy issues

e5fa44a

Merge branch 'develop' into module-map

0643432

Merge branch 'develop' into inline_subgraph

31a1bd8

code backup

b82c963

clang format

7c7fd44

Merge branch 'module-map' of github.com:ROCmSoftwarePlatform/AMDMIGra…

3ae87f5

…phX into inline_subgraph_try_removing_subgraph

fix problem related to inline subgraph

7fd995f

clang format

5477c73

Merge branch 'develop' of github.com:ROCmSoftwarePlatform/AMDMIGraphX…

3bc79ae

… into inline_subgraph

Merge branch 'inline_subgraph' of github.com:ROCmSoftwarePlatform/AMD…

8be989c

…MIGraphX into inline_subgraph

Merge branch 'develop' into inline_subgraph

330ec85

Merge branch 'develop' of github.com:ROCmSoftwarePlatform/AMDMIGraphX…

c0a66b5

… into loop_operator

code change backup

ae80fb7

clang format

95c44e6

fix error in compiling a program

f6bc9bd

Merge branch 'inline_subgraph' into loop_operator

849f5cf

Merge branch 'develop' into inline_subgraph

08c637b

fix review comments

8adab72

Merge branch 'inline_subgraph' of github.com:ROCmSoftwarePlatform/AMD…

76f978e

…MIGraphX into inline_subgraph

fix review comments

dc900f6

Merge branch 'develop' of github.com:ROCmSoftwarePlatform/AMDMIGraphX…

19a3530

… into loop_operator

fix review comments

384568b

Merge branch 'develop' of github.com:ROCmSoftwarePlatform/AMDMIGraphX…

3be1db1

… into inline_subgraph

Merge branch 'develop' of github.com:ROCmSoftwarePlatform/AMDMIGraphX…

c3284ab

… into loop_operator

Merge branch 'develop' of github.com:ROCmSoftwarePlatform/AMDMIGraphX…

0e1c56b

… into loop_operator

code backup

efffef3

clang format

3ee1d0b

Merge branch 'develop' of github.com:ROCmSoftwarePlatform/AMDMIGraphX…

9222446

… into loop_operator

code backup

300da15

clang format

d75081f

code cleanup

7e1760d

pfultz2 reviewed Sep 10, 2021

View reviewed changes

scxiao and others added 7 commits September 9, 2021 22:44

fix review comments

abbc09d

clang format

6e32d75

change the handling of parameter name handling in nested subgraph

a16d7a1

clang format

b822188

add a unit test for the run loop

0d143ee

clang format

d7b6883

Merge branch 'develop' into loop_operator

ec9fcf3

pfultz2 reviewed Sep 10, 2021

View reviewed changes

scxiao added 9 commits September 10, 2021 21:07

fix review comments

b0579d4

Merge branch 'loop_operator' of github.com:ROCmSoftwarePlatform/AMDMI…

e6be0a9

…GraphX into loop_operator

fixed a build error

72a5884

clang format

588e602

refine comments

8a32c6c

clang format

7f4778c

code cleanup

f95b7f0

clang format

b272f58

Merge branch 'loop_operator' of github.com:ROCmSoftwarePlatform/AMDMI…

2bb6e0d

…GraphX into loop_operator

pfultz2 approved these changes Sep 16, 2021

View reviewed changes

causten merged commit a275f59 into develop Sep 16, 2021

causten deleted the loop_operator branch September 16, 2021 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loop operator #853

Loop operator #853

scxiao commented Jun 9, 2021 •

edited

Loading

pfultz2 Sep 10, 2021

scxiao Sep 10, 2021

pfultz2 Sep 10, 2021

scxiao Sep 10, 2021

pfultz2 Sep 10, 2021

scxiao Sep 10, 2021

pfultz2 Sep 10, 2021

scxiao Sep 10, 2021

scxiao Sep 10, 2021

pfultz2 Sep 10, 2021

scxiao Sep 10, 2021

pfultz2 Sep 10, 2021

scxiao Sep 10, 2021

pfultz2 Sep 10, 2021

scxiao Sep 10, 2021

pfultz2 Sep 10, 2021

scxiao Sep 10, 2021

pfultz2 Sep 10, 2021

scxiao Sep 11, 2021

pfultz2 Sep 10, 2021

scxiao Sep 11, 2021

Loop operator #853

Loop operator #853

Conversation

scxiao commented Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scxiao commented Jun 9, 2021 •

edited

Loading