Skip to content

Conversation

@Hzfengsy
Copy link
Member

This PR adds support for simple dynamic-shape-aware fusion, which is the first step towards supporting dynamic shapes. The main changes are as follows:

  • Fix FuncStructInfo in well-formed checks
  • Renew symbolic var defs in fuse_ops to prevent malformed functions

cc @tqchen @MasterJH5574 @jinhongyii

@tvm-bot
Copy link
Collaborator

tvm-bot commented Mar 24, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

1 similar comment
@tvm-bot
Copy link
Collaborator

tvm-bot commented Mar 24, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

This PR adds support for simple dynamic-shape-aware fusion, which is the first step towards supporting dynamic shapes. The main changes are as follows:

- Fix FuncStructInfo in well-formed checks
- Renew symbolic var defs in fuse_ops to prevent malformed functions
@tqchen tqchen merged commit 23803b7 into apache:unity Mar 24, 2023
farshidsp pushed a commit to farshidsp/tvm that referenced this pull request Mar 27, 2023
This PR adds support for simple dynamic-shape-aware fusion, which is the first step towards supporting dynamic shapes. The main changes are as follows:

- Fix FuncStructInfo in well-formed checks
- Renew symbolic var defs in fuse_ops to prevent malformed functions
tqchen pushed a commit that referenced this pull request Apr 1, 2023
This PR adds support for simple dynamic-shape-aware fusion, which is the first step towards supporting dynamic shapes. The main changes are as follows:

- Fix FuncStructInfo in well-formed checks
- Renew symbolic var defs in fuse_ops to prevent malformed functions
tqchen pushed a commit that referenced this pull request Apr 1, 2023
This PR adds support for simple dynamic-shape-aware fusion, which is the first step towards supporting dynamic shapes. The main changes are as follows:

- Fix FuncStructInfo in well-formed checks
- Renew symbolic var defs in fuse_ops to prevent malformed functions
tqchen pushed a commit that referenced this pull request Apr 1, 2023
This PR adds support for simple dynamic-shape-aware fusion, which is the first step towards supporting dynamic shapes. The main changes are as follows:

- Fix FuncStructInfo in well-formed checks
- Renew symbolic var defs in fuse_ops to prevent malformed functions
@masahi
Copy link
Member

masahi commented Apr 3, 2023

Hi @Hzfengsy, it seems this change broke one of CUTLASS BYOC tests: pytest test_codegen_cutlass.py::test_matmul_offload[float16-x_shape17-y_shape17-False-none-none] -s.

If I remove SymbolicVarRenewMutator::Renew, the test works. I don't know why it creates an undefined sym var for a BYOC function. Can you take a look?

cc @junrushao @vinx13 @yelite

@Hzfengsy
Copy link
Member Author

Hzfengsy commented Apr 3, 2023

Hi @masahi, thanks for reporting the bug.

SymbolicVarRenewMutator::Renew works to re-define all symbolic vars to prevent the same symbolic var in different functions.

@Hzfengsy
Copy link
Member Author

Hzfengsy commented Apr 3, 2023

@masahi, After a deep dive, it's may not the issue of FuseOps, as the output is well-formed and pass the check. The error is reported at the pass RunCodegen

@masahi
Copy link
Member

masahi commented Apr 3, 2023

Yes, it might be a bug in RunCodegen. And IRs produced by with or without SymbolicVarRenewMutator::Renew look identical at least textually. So I'm confused what makes SymbolicVarRenewMutator::Renew break RunCodegen.

@masahi
Copy link
Member

masahi commented Apr 3, 2023

However, it does not update the out_sinfo of the body

Why not? Does this mean out_sinfo continue referring to an old sym var?

@Hzfengsy
Copy link
Member Author

Hzfengsy commented Apr 3, 2023

Why not? Does this mean out_sinfo refer to the old sym var?

Sorry, it updated. My bad

@masahi
Copy link
Member

masahi commented Apr 3, 2023

https://github.com/apache/tvm/blob/unity/src/relax/transform/run_codegen.cc#L105

Maybe we shouldn't use the original function's ret_struct_info there. After RunCodegen, it indeed references a dubious sym var:

@R.function                                                                                                                                          
def main(x: R.Tensor((1, 1, 16, 15), dtype="float16"), y: R.Tensor((3, 2, "a", 15, 2), dtype="float16")) -> R.Tensor((3, 2, "a", 16, 2), dtype="float
16"):                                                                     
    a = T.int64()                                                         
    a_1 = T.int64()   <- this shouldn't exisit                                                    
    with R.dataflow():                                                                                                                               
        gv = R.call_dps_packed("fused_relax_matmul_cutlass", (x, y), out_sinfo=R.Tensor((3, 2, a_1, 16, 2), dtype="float16"))                     
      R.output(gv)                                                      
    return   

@masahi
Copy link
Member

masahi commented Apr 3, 2023

@Hzfengsy Is this line in ExprMutator still correct?

https://github.com/apache/tvm/blob/unity/src/relax/ir/expr_functor.cc#L592

I think the mutated function would continue referring to the old sym var if the sym var is used in a return sinfo.

@Hzfengsy
Copy link
Member Author

Hzfengsy commented Apr 3, 2023

I think the mutated function would continue referring to the old sym var if the sym var is used in a return sinfo.

We need to update the ret_struct_info. However, I'm suprised that the undefined var in ret_sinfo is not reported by well-formed checker

@masahi
Copy link
Member

masahi commented Apr 3, 2023

I think this is a problem unique to BYOC. RunCodegen replaces a call to a function with an opaque call_dps_packed call. After RunCodegen runs, the original function is gone but we still need its ret_struct_info.

@masahi
Copy link
Member

masahi commented Apr 3, 2023

The fix has been added in the PR #14291

Hzfengsy pushed a commit to Hzfengsy/tvm that referenced this pull request Apr 6, 2023
In the last PR apache#14396, we enable
dynamic-shape-aware fusion in FuseOps. In this PR, we support the following
FuseTIR pass for simple cases.

This PR also fixes a minor compilation warning in `json_runtime.h`
Hzfengsy pushed a commit that referenced this pull request Apr 6, 2023
In the last PR #14396, we enable
dynamic-shape-aware fusion in FuseOps. In this PR, we support the following
FuseTIR pass for simple cases.

This PR also fixes a minor compilation warning in `json_runtime.h`
@Hzfengsy Hzfengsy deleted the dyn_aware_fusion branch November 5, 2023 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants