fix relay.build to not change the module argument in place

[t-vi]

This fixes the observation at
https://discuss.tvm.ai/t/tvm-relay-build-modifying-its-argument/6958

[t-vi]

@tqchen

[junrushao]

Thank you for the fix!

[junrushao]

If I understand the issue correctly, I think we need to call CopyOnWrite instead.

IRModule is actually defined with CopyOnWrite method here, the macro expands to this. What we need to do is just call its CopyOnWrite method to generate a unique copy of IRModule.

[t-vi]

Thank you for the feedback!

OK, so I changed it to call CopyOnWrite and it appears to work.

To understand this better, I got the re-instatiation from FunctionPassNode::operator() here:
https://github.com/apache/incubator-tvm/blob/99745a44407f2d1bd06b8c6a47e6c6c5239ec665/src/relay/ir/transform.cc#L123
is there a reason that isn't done via CopyOnWrite?
Superficially, it seems that the update is then
https://github.com/apache/incubator-tvm/blob/99745a44407f2d1bd06b8c6a47e6c6c5239ec665/src/relay/ir/transform.cc#L135
which would seem very similar to the Update done in the Optimize
https://github.com/apache/incubator-tvm/blob/99745a44407f2d1bd06b8c6a47e6c6c5239ec665/src/relay/backend/build_module.cc#L247
which itself just calls add
https://github.com/apache/incubator-tvm/blob/99745a44407f2d1bd06b8c6a47e6c6c5239ec665/src/ir/module.cc#L271-L273

Is something that could also use CopyOnWrite or is it not applicable there.

[t-vi]

I must admit I'm not understanding this very well yet, in particular in for the use of the new relay_module_ptr I get from CopyOnWrite and the original module.
Which one would I use for what after CopyOnWrite?

[t-vi]

Now it's not working anymore. :( I think I need a hint.

[junrushao]

Ooops let me take a closer look

[tqchen]

What I would do instead is to simply move relay_module.CopyOnWrite() inside params.size() != 0, which triggers copy if there is additional copy of relay module outside the function.

Note that for mods pass from python, CopyOnWrite always triggers a copy, unless we do mod._move() (moves the python side of the ref)

[t-vi]

@tqchen Thank you for the hint. I applied it, but I must admit I'm not entirely certain what's going on.

Do I need to call Update on the ptr returned by CopyOnWrite or on original module? Previously I called it on the original module, but 1) I'm not sure, 2) then the compiler complains about the return of CopyOnWrite being unused.

Also, it seems that CopyOnWrite + Update is certain to do the copy, is it?

[tqchen]

We don't have to use the ptr of CopyOnWrite in this case, the module also get updated to points to the new copy .

[junrushao]

@t-vi Here is what is going on: Internally, CopyOnWrite checks if the object (in our case, relay_module) is unique. If so, it does nothing; otherwise, it triggers a copy to make sure that the object is unique. Therefore, after calling CopyOnWrite, it makes sure that any change only happens locally, in our case, making relay.build don't change the input.

[t-vi]

Thanks for the explanation, I'll have to read the source a bit more, but now I can do so with a high-level sense of what's going on. 🙂

Happily the CI likes it better now, too.

[tqchen]

Thanks @t-vi for the contribution ! Thanks @junrushao1994 for reviewing