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

[DISCUSS] Naming of variables during transformation #42

Closed
ZihengJiang opened this issue Nov 18, 2021 · 1 comment
Closed

[DISCUSS] Naming of variables during transformation #42

ZihengJiang opened this issue Nov 18, 2021 · 1 comment

Comments

@ZihengJiang
Copy link
Contributor

We have lots of implicit agreement on variable naming, for example:

  • lv means local variable, while gv means global variable
  • sh means shape variable
  • storage means storage, alloc and tensor both mean allocated tensor
  • ...

Those naming conventions often are introduced by different transformation pass and the reflected property are often orthogonal, for example, a variable can be a local variable also a shape variable. Using those conventions together would make the final program a mess. For example:

Orignal program:

@tvm.script.ir_module
class Module:
    @relax.function
    def foo(x: Tensor[_, "float32"]) -> Tensor[_, _]:
        # block 0
        with relax.dataflow():
            sh = relax.call_packed("vm.builtin.shape_of", x)
            x0 = relax.match_shape(sh, (n, m))
            y = relax.call_dps((n, (m * 2)), "test.vm.tile", x)
            relax.output(y)
        return y

After lowering:

@tvm.script.ir_module
class Module:
    @tir.prim_func
    def shape_func(heap_1: tir.handle) -> None:
        # function attr dict
        tir.func_attr({"global_symbol": "shape_func"})
        H_1 = tir.match_buffer(heap_1, [tir.int64(4)], dtype="int64")
        # body
        tir.store(H_1.data, 2, tir.load("int64", H_1.data, 0) * (tir.load("int64", H_1.data, 1) * tir.int64(2)) * tir.int64(4), True)

    @tir.prim_func
    def shape_func1(heap_1: tir.handle) -> None:
        # function attr dict
        tir.func_attr({"global_symbol": "shape_func1"})
        H_1 = tir.match_buffer(heap_1, [tir.int64(4)], dtype="int64")
        # body
        tir.store(H_1.data, 0, tir.load("int64", H_1.data, 0), True)
        tir.store(H_1.data, 3, tir.load("int64", H_1.data, 1) * tir.int64(2), True)

    @relax.function
    def foo(x: Tensor[_, "float32"]) -> Tensor[_, _]:
        # block 0
        shape_heap: Tensor[(4,), "int64"] = relax.call_packed("vm.builtin.alloc_shape_heap", (4,))
        # block 1
        sh = relax.call_packed("vm.builtin.shape_of", x)
        gv = relax.vm.builtin.store_shape(sh, shape_heap, indices=[0, 1], attrs_type_key="relax.attrs.ShapeHeapAttrs")
        _ = shape_func(shape_heap)
        sh1 = relax.vm.builtin.load_shape(shape_heap, indices=[2], attrs_type_key="relax.attrs.ShapeHeapAttrs")
        storage = relax.vm.builtin.alloc_storage(sh1, device_type=1, dtype="float32", attrs_type_key="relax.attrs.AllocStorageAttrs")
        _1 = shape_func1(shape_heap)
        sh11 = relax.vm.builtin.load_shape(shape_heap, indices=[0, 3], attrs_type_key="relax.attrs.ShapeHeapAttrs")
        tensor = relax.vm.builtin.alloc_tensor(storage, sh11, offset=0, dtype="float32", attrs_type_key="relax.attrs.AllocTensorAttrs")
        alloc = tensor
        _2 = relax.call_packed("test.vm.tile", x, alloc)
        y = alloc
        # block 2
        return y

We might want to have a unified naming convention instead of creating one by one in each transformation to improve readability.

@altanh
Copy link
Collaborator

altanh commented Nov 19, 2021

summary of offline discussion during open source meeting:

  • We should definitely standardize the naming convention across passes at least. It would be good to keep some semantic hint in the name if possible, although this might need some further discussion to agree on a convention.
  • We should have a single NameTable that lives across the entire compilation pipeline, so that passes can query it without causing duplicate names (currently a new NameTable is created for every BlockBuilder, so each pass has a new NameTable).
  • People prefer the unique names to start with index 0, e.g. "x" -> "x0" (the first time gettin a unique name for "x").
  • There could still be potential for edge case unique name generation causing something like "x1" -> "x11". We should try to identify the causes and prevent this if possible.

MasterJH5574 pushed a commit to MasterJH5574/tlc-relax that referenced this issue Dec 13, 2022
vinx13 pushed a commit to vinx13/relax that referenced this issue Dec 14, 2022
MasterJH5574 pushed a commit to MasterJH5574/tlc-relax that referenced this issue Dec 24, 2022
MasterJH5574 added a commit to MasterJH5574/tlc-relax that referenced this issue Dec 24, 2022
 (tlc-pack#57)

* [FIX][Pass] concurrent modification in RemoveUnusedVars (tlc-pack#32)

* [Pass] Fix concurrent modification in RemoveUnusedVars

When running RemoveUnusedVars (i.e. remove_all_unused), in some cases the map users will raise Concurrent modification error. This commit fixed it by changing the logic to "iterate the map first and update it later".

* change the algorithm by store keys first

* Add an ICHECK before getting map value

* change the information of the ICHECK

* fix find include path (tlc-pack#42)

Co-authored-by: Hongyi Jin <[email protected]>

* [BUG] ExternFunc is not considered in attach_global_symbol.cc (tlc-pack#48)

* [Cherry-Pick] Minor fix for TaskScheduler and VerifyGPUCode (tlc-pack#54)

* [Fix] Task scheduler error prompt upon build/run failure (#13601)

* [Fix] Use proper target in VerifyGPUCode (#13548)

Previously, the VerifyGPUCode post-processor uses hardcoded target `Target("cuda")` for applying pass LowerIntrin. This is a bit problematic since the actual target can be other GPU target (e.g., Metal). Therefore, this PR changes the hardcoded target to be the actual target.

Co-authored-by: Chaosfan <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Bohan Hou <[email protected]>
MasterJH5574 added a commit to MasterJH5574/tlc-relax that referenced this issue Dec 30, 2022
 (tlc-pack#57)

* [FIX][Pass] concurrent modification in RemoveUnusedVars (tlc-pack#32)

* [Pass] Fix concurrent modification in RemoveUnusedVars

When running RemoveUnusedVars (i.e. remove_all_unused), in some cases the map users will raise Concurrent modification error. This commit fixed it by changing the logic to "iterate the map first and update it later".

* change the algorithm by store keys first

* Add an ICHECK before getting map value

* change the information of the ICHECK

* fix find include path (tlc-pack#42)

Co-authored-by: Hongyi Jin <[email protected]>

* [BUG] ExternFunc is not considered in attach_global_symbol.cc (tlc-pack#48)

* [Cherry-Pick] Minor fix for TaskScheduler and VerifyGPUCode (tlc-pack#54)

* [Fix] Task scheduler error prompt upon build/run failure (#13601)

* [Fix] Use proper target in VerifyGPUCode (#13548)

Previously, the VerifyGPUCode post-processor uses hardcoded target `Target("cuda")` for applying pass LowerIntrin. This is a bit problematic since the actual target can be other GPU target (e.g., Metal). Therefore, this PR changes the hardcoded target to be the actual target.

Co-authored-by: Chaosfan <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Bohan Hou <[email protected]>
@tqchen tqchen closed this as completed Jan 6, 2023
vinx13 pushed a commit to vinx13/relax that referenced this issue Jan 9, 2023
 (tlc-pack#57)

* [FIX][Pass] concurrent modification in RemoveUnusedVars (tlc-pack#32)

* [Pass] Fix concurrent modification in RemoveUnusedVars

When running RemoveUnusedVars (i.e. remove_all_unused), in some cases the map users will raise Concurrent modification error. This commit fixed it by changing the logic to "iterate the map first and update it later".

* change the algorithm by store keys first

* Add an ICHECK before getting map value

* change the information of the ICHECK

* fix find include path (tlc-pack#42)

Co-authored-by: Hongyi Jin <[email protected]>

* [BUG] ExternFunc is not considered in attach_global_symbol.cc (tlc-pack#48)

* [Cherry-Pick] Minor fix for TaskScheduler and VerifyGPUCode (tlc-pack#54)

* [Fix] Task scheduler error prompt upon build/run failure (#13601)

* [Fix] Use proper target in VerifyGPUCode (#13548)

Previously, the VerifyGPUCode post-processor uses hardcoded target `Target("cuda")` for applying pass LowerIntrin. This is a bit problematic since the actual target can be other GPU target (e.g., Metal). Therefore, this PR changes the hardcoded target to be the actual target.

Co-authored-by: Chaosfan <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Bohan Hou <[email protected]>
MasterJH5574 added a commit to MasterJH5574/tlc-relax that referenced this issue Jan 13, 2023
 (tlc-pack#57)

* [FIX][Pass] concurrent modification in RemoveUnusedVars (tlc-pack#32)

* [Pass] Fix concurrent modification in RemoveUnusedVars

When running RemoveUnusedVars (i.e. remove_all_unused), in some cases the map users will raise Concurrent modification error. This commit fixed it by changing the logic to "iterate the map first and update it later".

* change the algorithm by store keys first

* Add an ICHECK before getting map value

* change the information of the ICHECK

* fix find include path (tlc-pack#42)

Co-authored-by: Hongyi Jin <[email protected]>

* [BUG] ExternFunc is not considered in attach_global_symbol.cc (tlc-pack#48)

* [Cherry-Pick] Minor fix for TaskScheduler and VerifyGPUCode (tlc-pack#54)

* [Fix] Task scheduler error prompt upon build/run failure (#13601)

* [Fix] Use proper target in VerifyGPUCode (#13548)

Previously, the VerifyGPUCode post-processor uses hardcoded target `Target("cuda")` for applying pass LowerIntrin. This is a bit problematic since the actual target can be other GPU target (e.g., Metal). Therefore, this PR changes the hardcoded target to be the actual target.

Co-authored-by: Chaosfan <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Bohan Hou <[email protected]>
MasterJH5574 added a commit to MasterJH5574/tlc-relax that referenced this issue Jan 16, 2023
 (tlc-pack#57)

* [FIX][Pass] concurrent modification in RemoveUnusedVars (tlc-pack#32)

* [Pass] Fix concurrent modification in RemoveUnusedVars

When running RemoveUnusedVars (i.e. remove_all_unused), in some cases the map users will raise Concurrent modification error. This commit fixed it by changing the logic to "iterate the map first and update it later".

* change the algorithm by store keys first

* Add an ICHECK before getting map value

* change the information of the ICHECK

* fix find include path (tlc-pack#42)

Co-authored-by: Hongyi Jin <[email protected]>

* [BUG] ExternFunc is not considered in attach_global_symbol.cc (tlc-pack#48)

* [Cherry-Pick] Minor fix for TaskScheduler and VerifyGPUCode (tlc-pack#54)

* [Fix] Task scheduler error prompt upon build/run failure (#13601)

* [Fix] Use proper target in VerifyGPUCode (#13548)

Previously, the VerifyGPUCode post-processor uses hardcoded target `Target("cuda")` for applying pass LowerIntrin. This is a bit problematic since the actual target can be other GPU target (e.g., Metal). Therefore, this PR changes the hardcoded target to be the actual target.

Co-authored-by: Chaosfan <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Bohan Hou <[email protected]>
vinx13 pushed a commit to vinx13/relax that referenced this issue Jan 31, 2023
 (tlc-pack#57)

* [FIX][Pass] concurrent modification in RemoveUnusedVars (tlc-pack#32)

* [Pass] Fix concurrent modification in RemoveUnusedVars

When running RemoveUnusedVars (i.e. remove_all_unused), in some cases the map users will raise Concurrent modification error. This commit fixed it by changing the logic to "iterate the map first and update it later".

* change the algorithm by store keys first

* Add an ICHECK before getting map value

* change the information of the ICHECK

* fix find include path (tlc-pack#42)

Co-authored-by: Hongyi Jin <[email protected]>

* [BUG] ExternFunc is not considered in attach_global_symbol.cc (tlc-pack#48)

* [Cherry-Pick] Minor fix for TaskScheduler and VerifyGPUCode (tlc-pack#54)

* [Fix] Task scheduler error prompt upon build/run failure (#13601)

* [Fix] Use proper target in VerifyGPUCode (#13548)

Previously, the VerifyGPUCode post-processor uses hardcoded target `Target("cuda")` for applying pass LowerIntrin. This is a bit problematic since the actual target can be other GPU target (e.g., Metal). Therefore, this PR changes the hardcoded target to be the actual target.

Co-authored-by: Chaosfan <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Bohan Hou <[email protected]>
vinx13 pushed a commit to vinx13/relax that referenced this issue Jan 31, 2023
 (tlc-pack#57)

* [FIX][Pass] concurrent modification in RemoveUnusedVars (tlc-pack#32)

* [Pass] Fix concurrent modification in RemoveUnusedVars

When running RemoveUnusedVars (i.e. remove_all_unused), in some cases the map users will raise Concurrent modification error. This commit fixed it by changing the logic to "iterate the map first and update it later".

* change the algorithm by store keys first

* Add an ICHECK before getting map value

* change the information of the ICHECK

* fix find include path (tlc-pack#42)

Co-authored-by: Hongyi Jin <[email protected]>

* [BUG] ExternFunc is not considered in attach_global_symbol.cc (tlc-pack#48)

* [Cherry-Pick] Minor fix for TaskScheduler and VerifyGPUCode (tlc-pack#54)

* [Fix] Task scheduler error prompt upon build/run failure (#13601)

* [Fix] Use proper target in VerifyGPUCode (#13548)

Previously, the VerifyGPUCode post-processor uses hardcoded target `Target("cuda")` for applying pass LowerIntrin. This is a bit problematic since the actual target can be other GPU target (e.g., Metal). Therefore, this PR changes the hardcoded target to be the actual target.

Co-authored-by: Chaosfan <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Bohan Hou <[email protected]>
vinx13 pushed a commit to vinx13/relax that referenced this issue Feb 8, 2023
 (tlc-pack#57)

* [FIX][Pass] concurrent modification in RemoveUnusedVars (tlc-pack#32)

* [Pass] Fix concurrent modification in RemoveUnusedVars

When running RemoveUnusedVars (i.e. remove_all_unused), in some cases the map users will raise Concurrent modification error. This commit fixed it by changing the logic to "iterate the map first and update it later".

* change the algorithm by store keys first

* Add an ICHECK before getting map value

* change the information of the ICHECK

* fix find include path (tlc-pack#42)

Co-authored-by: Hongyi Jin <[email protected]>

* [BUG] ExternFunc is not considered in attach_global_symbol.cc (tlc-pack#48)

* [Cherry-Pick] Minor fix for TaskScheduler and VerifyGPUCode (tlc-pack#54)

* [Fix] Task scheduler error prompt upon build/run failure (#13601)

* [Fix] Use proper target in VerifyGPUCode (#13548)

Previously, the VerifyGPUCode post-processor uses hardcoded target `Target("cuda")` for applying pass LowerIntrin. This is a bit problematic since the actual target can be other GPU target (e.g., Metal). Therefore, this PR changes the hardcoded target to be the actual target.

Co-authored-by: Chaosfan <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Bohan Hou <[email protected]>
MasterJH5574 added a commit to MasterJH5574/tlc-relax that referenced this issue Feb 12, 2023
 (tlc-pack#57)

* [FIX][Pass] concurrent modification in RemoveUnusedVars (tlc-pack#32)

* [Pass] Fix concurrent modification in RemoveUnusedVars

When running RemoveUnusedVars (i.e. remove_all_unused), in some cases the map users will raise Concurrent modification error. This commit fixed it by changing the logic to "iterate the map first and update it later".

* change the algorithm by store keys first

* Add an ICHECK before getting map value

* change the information of the ICHECK

* fix find include path (tlc-pack#42)

Co-authored-by: Hongyi Jin <[email protected]>

* [BUG] ExternFunc is not considered in attach_global_symbol.cc (tlc-pack#48)

* [Cherry-Pick] Minor fix for TaskScheduler and VerifyGPUCode (tlc-pack#54)

* [Fix] Task scheduler error prompt upon build/run failure (#13601)

* [Fix] Use proper target in VerifyGPUCode (#13548)

Previously, the VerifyGPUCode post-processor uses hardcoded target `Target("cuda")` for applying pass LowerIntrin. This is a bit problematic since the actual target can be other GPU target (e.g., Metal). Therefore, this PR changes the hardcoded target to be the actual target.

Co-authored-by: Chaosfan <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Bohan Hou <[email protected]>
vinx13 pushed a commit to vinx13/relax that referenced this issue Feb 13, 2023
 (tlc-pack#57)

* [FIX][Pass] concurrent modification in RemoveUnusedVars (tlc-pack#32)

* [Pass] Fix concurrent modification in RemoveUnusedVars

When running RemoveUnusedVars (i.e. remove_all_unused), in some cases the map users will raise Concurrent modification error. This commit fixed it by changing the logic to "iterate the map first and update it later".

* change the algorithm by store keys first

* Add an ICHECK before getting map value

* change the information of the ICHECK

* fix find include path (tlc-pack#42)

Co-authored-by: Hongyi Jin <[email protected]>

* [BUG] ExternFunc is not considered in attach_global_symbol.cc (tlc-pack#48)

* [Cherry-Pick] Minor fix for TaskScheduler and VerifyGPUCode (tlc-pack#54)

* [Fix] Task scheduler error prompt upon build/run failure (#13601)

* [Fix] Use proper target in VerifyGPUCode (#13548)

Previously, the VerifyGPUCode post-processor uses hardcoded target `Target("cuda")` for applying pass LowerIntrin. This is a bit problematic since the actual target can be other GPU target (e.g., Metal). Therefore, this PR changes the hardcoded target to be the actual target.

Co-authored-by: Chaosfan <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Bohan Hou <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants