Skip to content
This repository has been archived by the owner on May 22, 2023. It is now read-only.

[Transform] RewriteDataflowReshape to op and VMBuiltinLower handling #415

Conversation

MasterJH5574
Copy link
Collaborator

As discussed in #407 (comment), we update the behavior of pass RewriteDataflowReshape.

In short, prior to this PR, the pass transforms calls of reshape PrimFunc in dataflow blocks to direct calls of runtime packed func “vm.builtin.reshape.” The consequence of this behavior is that the memory planning pass has to check the reshape op by string comparison of ExternFunc.global_symbol, which is not ideal.

Therefore, this PR changes the RewriteDataflowReshape’s behavior, transforming calls of reshape PrimFunc to our high-level reshape op “relax.reshape,” and let the VMBuiltinLower pass to lowers the op to calls of “vm.builtin.reshape.”

As discussed in tlc-pack#407 (comment),
we update the behavior of pass RewriteDataflowReshape.

In short, prior to this PR, the pass transforms calls of reshape
PrimFunc in dataflow blocks to direct calls of runtime packed func
“vm.builtin.reshape.” The consequence of this behavior is that the
memory planning pass has to check the reshape op by string comparison
of `ExternFunc.global_symbol`, which is not ideal.

Therefore, this PR changes the RewriteDataflowReshape’s behavior,
transforming calls of reshape PrimFunc to our high-level reshape op
“relax.reshape,” and let the VMBuiltinLower pass to lowers the op to
calls of “vm.builtin.reshape.”
@MasterJH5574 MasterJH5574 force-pushed the relax-dev/2023-02-08-rewrite-dataflow-reshape-to-op branch from 5ff6b63 to 5c041c5 Compare February 8, 2023 15:26
@masahi
Copy link
Contributor

masahi commented Feb 8, 2023

Unrelated, but the following line that assumes the callee of call_tir is GlobalVar is incorrect, since it can also be ExternFunc after RunCodegen pass in the BYOC flow.

https://github.com/tlc-pack/relax/blob/relax/src/relax/transform/rewrite_dataflow_reshape.cc#L71

Can you add a check call->args[0]->IsInstance<ExternFuncNode>() at L68?

@MasterJH5574
Copy link
Collaborator Author

MasterJH5574 commented Feb 8, 2023

Unrelated, but the following line that assumes the callee of call_tir is GlobalVar is incorrect, since it can also be ExternFunc after RunCodegen pass in the BYOC flow.

https://github.com/tlc-pack/relax/blob/relax/src/relax/transform/rewrite_dataflow_reshape.cc#L71

Can you add a check call->args[0]->IsInstance<ExternFuncNode>() at L68?

@masahi Thanks! Sorry it was my negligence. Will add in the next commit.

@MasterJH5574
Copy link
Collaborator Author

@masahi Updated :-)

@slyubomirsky
Copy link
Collaborator

We should probably document when in compilation this rewrite (and any other passes whose order is important) should happen, since there will be dependencies on it (like #407).

@MasterJH5574 MasterJH5574 force-pushed the relax-dev/2023-02-08-rewrite-dataflow-reshape-to-op branch from 1519bb9 to 3c79b61 Compare February 8, 2023 23:08
@MasterJH5574
Copy link
Collaborator Author

We should probably document when in compilation this rewrite (and any other passes whose order is important) should happen, since there will be dependencies on it (like #407).

@slyubomirsky Thanks! Added one note here

* \note The pass is applied at the first stage of Relax VM build, before
* rewriting call_tir, as this pass requires dataflow information.
*/
Notes
-----
The pass is applied at the first stage of Relax VM build, before
rewriting call_tir, as this pass requires dataflow information.
"""
return _ffi_api.RewriteDataflowReshape() # type: ignore

@tqchen tqchen merged commit 1f84c7b into tlc-pack:relax Feb 9, 2023
junrushao pushed a commit that referenced this pull request Feb 9, 2023
…415)

* [Transform] RewriteDataflowReshape to op and VMBuiltinLower handling

Priior to this PR, the pass transforms calls of reshape
PrimFunc in dataflow blocks to direct calls of runtime packed func
“vm.builtin.reshape.” The consequence of this behavior is that the
memory planning pass has to check the reshape op by string comparison
of `ExternFunc.global_symbol`, which is not ideal.

Therefore, this PR changes the RewriteDataflowReshape’s behavior,
transforming calls of reshape PrimFunc to our high-level reshape op
“relax.reshape,” and let the VMBuiltinLower pass to lowers the op to
calls of “vm.builtin.reshape.”
junrushao pushed a commit that referenced this pull request Feb 9, 2023
…415)

* [Transform] RewriteDataflowReshape to op and VMBuiltinLower handling

Priior to this PR, the pass transforms calls of reshape
PrimFunc in dataflow blocks to direct calls of runtime packed func
“vm.builtin.reshape.” The consequence of this behavior is that the
memory planning pass has to check the reshape op by string comparison
of `ExternFunc.global_symbol`, which is not ideal.

Therefore, this PR changes the RewriteDataflowReshape’s behavior,
transforming calls of reshape PrimFunc to our high-level reshape op
“relax.reshape,” and let the VMBuiltinLower pass to lowers the op to
calls of “vm.builtin.reshape.”
spectrometerHBH referenced this pull request in spectrometerHBH/relax Feb 9, 2023
…(#415)

* [Transform] RewriteDataflowReshape to op and VMBuiltinLower handling

Priior to this PR, the pass transforms calls of reshape
PrimFunc in dataflow blocks to direct calls of runtime packed func
“vm.builtin.reshape.” The consequence of this behavior is that the
memory planning pass has to check the reshape op by string comparison
of `ExternFunc.global_symbol`, which is not ideal.

Therefore, this PR changes the RewriteDataflowReshape’s behavior,
transforming calls of reshape PrimFunc to our high-level reshape op
“relax.reshape,” and let the VMBuiltinLower pass to lowers the op to
calls of “vm.builtin.reshape.”
spectrometerHBH referenced this pull request in spectrometerHBH/relax Feb 11, 2023
…(#415)

* [Transform] RewriteDataflowReshape to op and VMBuiltinLower handling

Priior to this PR, the pass transforms calls of reshape
PrimFunc in dataflow blocks to direct calls of runtime packed func
“vm.builtin.reshape.” The consequence of this behavior is that the
memory planning pass has to check the reshape op by string comparison
of `ExternFunc.global_symbol`, which is not ideal.

Therefore, this PR changes the RewriteDataflowReshape’s behavior,
transforming calls of reshape PrimFunc to our high-level reshape op
“relax.reshape,” and let the VMBuiltinLower pass to lowers the op to
calls of “vm.builtin.reshape.”
@MasterJH5574
Copy link
Collaborator Author

Unrelated, but the following line that assumes the callee of call_tir is GlobalVar is incorrect, since it can also be ExternFunc after RunCodegen pass in the BYOC flow.

https://github.com/tlc-pack/relax/blob/relax/src/relax/transform/rewrite_dataflow_reshape.cc#L71

Can you add a check call->args[0]->IsInstance<ExternFuncNode>() at L68?

@masahi Hi Masa. Sorry that I didn’t think too much on your comment before. One quick question: why we use call_tir to call an ExternFunc, while not directly use the ExternFunc as CallNode::op? call_tir is supposed to call TIR PrimFunc IMO. And if we want to call some ExternFunc, we can directly call that ExternFunc via Call(op=extern_func, args=my_args, ...).

@masahi
Copy link
Contributor

masahi commented Feb 11, 2023

I don't know, and I wondered too. cc @sunggg who wrote the code below.

static const Op& call_op = Op::Get("relax.call_tir");

@MasterJH5574
Copy link
Collaborator Author

MasterJH5574 commented Feb 11, 2023

@masahi I just got a quick answer.

Here we use call_tir calling ExternFunc for call_dps_packed. This means the interface of the ExternFunc is in DPS style, and thus when we use call_tir to call it, we write a normal call instead of a DPS call, and don’t need to allocate the result memory in ahead.

We may need a dedicated op for call_dps_packed (#430), as using call_tir both normal call_tir and call_dps_packed is too much confusing. Nevertheless, we can leave it as it is now. Sorry for the bothering.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants