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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to