Re: [PR] [SLM][Bugfix] Output debug functions as impure [tvm]

["slyubomirsky (via GitHub)" <gi...@apache.org>]

slyubomirsky commented on code in PR #16687:
URL: https://github.com/apache/tvm/pull/16687#discussion_r1525679154


##########
python/tvm/script/ir_builder/relax/ir.py:
##########
@@ -222,7 +222,7 @@ def to_vdevice(data: Expr, dst_vdevice: Union[py_str, VDevice]) -> Expr:
 ############################### Function ################################
 
 
-def function(is_pure: bool = True, is_private: bool = False) -> frame.FunctionFrame:
+def function(is_pure: Optional[bool] = None, is_private: bool = False) -> frame.FunctionFrame:

Review Comment:
   The docs for `is_pure` should mention that it's inferred.



##########
src/relax/ir/expr.cc:
##########
@@ -473,6 +473,23 @@ Function::Function(Array<Var> params, Expr body, Optional<StructInfo> ret_struct
     ret_struct_info = body_sinfo;
   }
 
+  bool is_pure = [&]() -> bool {

Review Comment:
   How would this behave in the recursive or mutually recursive case? That was the reason for not inferring purity in the first place. Detecting those cases and warning the user that they need to be explicitly annotated would be a reasonable approach



##########
include/tvm/relax/expr.h:
##########
@@ -983,16 +983,21 @@ class FunctionNode : public BaseFuncNode {
 class Function : public BaseFunc {
  public:
   TVM_DLL explicit Function(Array<Var> params, Expr body, Optional<StructInfo> ret_struct_info,
-                            bool is_pure = true, DictAttrs attrs = NullValue<DictAttrs>(),
-                            Span span = Span());
+                            bool is_pure, DictAttrs attrs = NullValue<DictAttrs>(),
+                            Span span = Span())
+      : Function(params, body, ret_struct_info, Optional<Bool>(Bool(is_pure)), attrs, span) {}
+
+  TVM_DLL explicit Function(Array<Var> params, Expr body,
+                            Optional<StructInfo> ret_struct_info = NullOpt,
+                            Optional<Bool> is_pure = NullOpt,
+                            DictAttrs attrs = NullValue<DictAttrs>(), Span span = Span());
 
   /*!
    * \brief Mimics the constructor but without body Expr.
-   * \note ret_struct_info is required, since it can not deduced by the body.
+   * \note `ret_struct_info` and `is_pure` are required, since it can not deduced by the body.

Review Comment:
   ```suggestion
      * \note `ret_struct_info` and `is_pure` are required, since they cannot be deduced from the body.
   ```
   
   Might as well correct the typo too



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