tqchen commented on code in PR #16090:
URL: https://github.com/apache/tvm/pull/16090#discussion_r1388080958


##########
python/tvm/relax/block_builder.py:
##########
@@ -661,12 +661,20 @@ def normalize(self, expr: Expr) -> Expr:
     def get(self) -> tvm.IRModule:
         """Return the IRModule being built.
 
+        Note this call may invalidate global vars previously returned by this 
builder
+        (see tvm.relax.transform.NormalizeGlobalVar), so it is recommended to 
call this function
+        once at the end of the build process.
+
         Returns
         -------
         ret : tvm.IRModule
             An IRModule with Relax and TIR functions being built.
         """
-        return _ffi_api.BlockBuilderGetContextIRModule(self)  # type: ignore
+        # avoid circular import
+        from .transform import NormalizeGlobalVar
+
+        mod = _ffi_api.BlockBuilderGetContextIRModule(self)  # type: ignore

Review Comment:
   I discussed it with @Ubospica and think that we should not turn it on by 
default. Because there are not too many cases we create new public functions, 
and renormlaization can be costly. We should however add wellform check to 
ensure normal passes do not generate code that violate this assumption
   
   
   Maybe we do not make it part of `BlockBuilder.get()` but instead add a new 
function `BlockBuilder.finalize()` and add remark that it will also normalizes 
the calls. 



##########
python/tvm/relax/block_builder.py:
##########
@@ -661,12 +661,20 @@ def normalize(self, expr: Expr) -> Expr:
     def get(self) -> tvm.IRModule:
         """Return the IRModule being built.
 
+        Note this call may invalidate global vars previously returned by this 
builder
+        (see tvm.relax.transform.NormalizeGlobalVar), so it is recommended to 
call this function
+        once at the end of the build process.
+
         Returns
         -------
         ret : tvm.IRModule
             An IRModule with Relax and TIR functions being built.
         """
-        return _ffi_api.BlockBuilderGetContextIRModule(self)  # type: ignore
+        # avoid circular import
+        from .transform import NormalizeGlobalVar
+
+        mod = _ffi_api.BlockBuilderGetContextIRModule(self)  # type: ignore

Review Comment:
   I think that we should not turn it on by default. Because there are not too 
many cases we create new public functions, and renormlaization can be costly. 
We should however add wellform check to ensure normal passes do not generate 
code that violate this assumption
   
   
   Maybe we do not make it part of `BlockBuilder.get()` but instead add a new 
function `BlockBuilder.finalize()` and add remark that it will also normalizes 
the calls. 



-- 
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