-
Notifications
You must be signed in to change notification settings - Fork 3.7k
support returned function in relay.build #10502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Don't know who to cc. But in TVM community, I had some discussion with @masahi on this problem. So... could you please take a look on my fix? Don't know if it violates some basic principle of TVM. |
|
I don't think returning a function is something we want to support in graph runtime. What do you want to do with it? |
@masahi Hi. Thanks for your reply. I make this change from the perspective of TVM users. If returning functions is allowed when we play with relay, then I think it should be compiled with If you agree with the latter, I can change back and add an ICheck |
|
I mean, have you done anything with the returned function from I don't think we can return a function to python and do anything with it. |
I haven't. I get your point. But I think adding a ICheck to give users a reasonable warning message instead of directly showing segmentation fault is reasonable. If you agree, I will add one. |
masahi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think adding a ICheck to give users a reasonable warning message instead of directly showing segmentation fault is reasonable
Sure, see my comment below.
| ICHECK(op->GetAttr<String>(attr::kCompiler).defined()) | ||
| << "Only functions supported by custom codegen"; | ||
| // ICHECK(op->GetAttr<String>(attr::kCompiler).defined()) | ||
| // << "Only functions supported by custom codegen"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this diff
| return 0; | ||
| } | ||
| auto tensor_type = expr_type.as<TensorTypeNode>(); | ||
| auto shape = tensor_type->shape; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the above diff and add ICHECK(tensor_type).
* fix InferType bug * fix InferType related bug * support returned function in relay.build * support returned function in relay.build * support returned function in relay.build * support returned function in relay.build * add warning about function returning function
* fix InferType bug * fix InferType related bug * support returned function in relay.build * support returned function in relay.build * support returned function in relay.build * support returned function in relay.build * add warning about function returning function
When
relay.builda IRModule in which a function whose returned value includes another function exists, TVM will crash with segmentation fault. Initially I assume this is the behavior disllowed by TVM, but intest_analysis_basic_block_normal_form.py, the following function shows the tolerance of it.So I write the following script with simpler structure to fuzz TVM:
The relay IR is easy, only including a function whose returned value is another function
As expected, segmentation fault showed up.
By bug localization, I find this problem is caused during memory allocation in graph_executor_codegen. More concretely, function
CalculateRelayExprSizeBytesforgets the situation where the type returned variable could be of FuncType and directly turns the typenode into TensorTypeNode. Thenauto shape = tensor_type->shape;triggers the bug.