[GitHub] [tvm-rfcs] gigiblender commented on a diff in pull request #84: [RFC] Name mangling in IRModules

[GitBox <gi...@apache.org>]

gigiblender commented on code in PR #84:
URL: https://github.com/apache/tvm-rfcs/pull/84#discussion_r913137711


##########
rfcs/0077_name_mangling_ir_modules.md:
##########
@@ -0,0 +1,102 @@
+- Feature Name: name_mangling_ir_modules
+- Start Date: 2022-06-29
+- RFC PR: [apache/tvm-rfcs#xxxx](https://github.com/apache/tvm-rfcs/pull/xxxx)
+- GitHub Issue: [apache/tvm#0000](https://github.com/apache/tvm/issues/0000)
+
+# Summary
+[summary]: #summary
+
+This RFC proposes a clean-up of the current name mangling strategy.
+
+# Motivation
+[motivation]: #motivation
+
+One reason for this RFC is that currently, it is difficult to know at various points in the compiler whether a `name_hint` is final or has yet to be mangled.  Name mangling is performed in various places:
+
+- `tvm::runtime::get_name_mangled` prefixes a name (usually module name) with a function name and is called from
+    - the AOT executor to create the main function and set the "`global_symbol"` attribute. It is also used to obtain the main function name and run it. See `AOTExecutorCodegen::CreateMainFunc` and `AOTExecutorCodegen::Run`
+    - During TE lowering in `TECompilerImpl::Lower`.
+    - In the `source_module.cc` to perform codegen for the C runtime.
+    - In the `NameMangleExtFuncs::Run`, name mangling is applied to all the module functions before AOT codegen.
+- `tvm::relay::tec::GetUniqueName` is used to avoid conflicts between multiple variables/functions that have the same name.
+    - in the `TECompilerImpl::LowerInternal` and `LowerShape`.
+
+Additionally, multiple `GlobalVars` having the same name are created throughout the code. In `TECompilerImpl::LowerInternal`, they are reduced to single values.
+
+This RFC aims to unify the creation of unique `GlobalVars` and refactor the current `GlobalVar` name mangling to be done through a single entity. 
+
+# Guide-level explanation
+[guide-level-explanation]: #guide-level-explanation
+
+The changes are internal to TVM. 
+The code writer is expected to avoid as much as possible calling the constructor of `GlobalVar` and instead use a `NameSupply` to generate one. 
+The `NameSupply` is constructed by passing a `String prefix` (usually a module name) that is to be prepended to the `GlobalVars` generated: `auto name_supply = NameSupply::NameSupplyWithPrefix(mod_name);`
+
+A `NameSupply` can be attached to an `IRModule`. The `IRModule` acts a carrier for the `NameSupply` through compiler passes. 
+The attached `NameSupply` can then be retrieved from the `IRModule` when, for example, a pass generates new `GlobalVars`.
+
+The `NameSupply` contains two methods to provide a `GlobalVar`:
+
+- `GlobalVar UniqueGlobalFor(String name, bool add_prefix)` performs a cache lookup and returns a `GlobalVar`. If a miss occurs, a new `GlobalVar` is created, inserted into the cache, and returned. The `add_prefix` boolean defaults to `true`. If it is `false`, then the `prefix_` field will not be added to the `GlobalVar`.
+- `GlobalVar FreshGlobal(const String name, bool add_prefix)` guarantees to return a newly constructed `GlobalVar` that is guaranteed not to conflict by name with other `GlobalVars` generated by the same `NameSupply` object. 
+The functionality of `add_prefix` is as described above.
+
+The name mangling in case of conflict is currently performed by appending `_1_2_3..._n` to the `name` parameter. 
+A potential improvement would be to treat the integer suffix 
+separately from the string prefix. 
+The following text snippet provides the general idea of mangling improvement:
+```
+x = get_next_name("fun12", existing_names=["func12", "func13", "func4"])
+assert x == "func14"
+``` 
+This can be useful to avoid the current confusion caused by having multiple names in form:
+```
+func_1
+func_1_2
+func_1_2_3
+```
+
+Additionally, the `NameSupply` provides static methods to perform mangling on a string. For example, `String Get_name_mangled(const String& module_name, const String& name)` can be called to prefix a module name to a name. The AOT executor frequently calls this utility with predefined names.
+
+```
+NameSupply name_supply = NameSupply::NameSupplyWithPrefix(mod_name);
+..
+..
+// Will perform a lookup based on candidate_name and return a GlobalVar from the cache if found. Otherwise, a new GlobalVar.
+GlobalVar prim_fn_var = name_supply->UniqueGlobalFor(candidate_name_);
+// will always print "mod_name_candidate_name_"
+std::cout << prim_fn_var->name_hint;
+..
+..
+// Will always return a new GlobalVar with "name_hint" mangled.
+GlobalVar prim_fn_gvar = name_supply->FreshGlobal(candidate_name);
+// Will print "mod_name_candidate_name_0_1_2_3..n" depending on many times this name was used with "name_supply".
+std::cout << prim_fn_gvar->name_hint;
+```
+
+# Reference-level explanation
+[reference-level-explanation]: #reference-level-explanation
+
+The `NameSupply` will be implemented as a class in TVM following the usual design, extending `Object` and `ObjectRef`, respectively.
+As mentioned, the `NameSupply` will contain an internal map of `String -> GlobalVar` used to perform lookups by `UniqueGlobalFor`. Additionally, a map of `String -> Int` will be used to provide unique names for `FreshGlobal`.
+
+Additional refactorings to the compiler will be performed as part of this RFC. For example, the `GlobalVar` deduplication in `TECompilerImpl::LowerInternal` will be removed by changing the signature of `tvm::LowerSchedule`.
+
+# Drawbacks
+[drawbacks]: #drawbacks
+
+- There might be cases when creating new `GlobalVars` without using a `NameSupply` might be needed. For example, in `IRModule::FromExprInContext`, the expression might already be annotated with a global symbol.
+We must be sure that the `NameSupply` interacts well with the `global_symbol` attribute. 
+- Ensuring that the `NameSupply` is always used when possible is not enforceable.
+
+# **Rationale and alternatives**
+An alternative way to unify name supplying and mangling in IR modules is to create a compiler pass that does the job. 
+The benefit of this is that the implementation would not be very intrusive. The downside is that it could only address name mangling inside IRModules. 
+There are cases when GlobalVars are created outside an IRModule, only to be later assigned to it. 
+
+# Future possibilities
+[future-possibilities]: #future-possibilities
+
+Currently, the `NameSupply` cannot be accessed through FFI.

Review Comment:
   Agreed



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org