Skip to content
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

[Bug] fuse_ops takes link_params from executor instead of target #10390

Closed
kparzysz-quic opened this issue Feb 25, 2022 · 6 comments
Closed

[Bug] fuse_ops takes link_params from executor instead of target #10390

kparzysz-quic opened this issue Feb 25, 2022 · 6 comments

Comments

@kparzysz-quic
Copy link
Contributor

kparzysz-quic commented Feb 25, 2022

Follow-up to the discussion in PR8509.

Resnet50/float32 model crashes during compilation on Hexagon: changes from the PR enable code execution path that leads to an assertion in function Array<te::Tensor> ScheduleBuilder::VisitExpr_(const ConstantNode* op) due to not handling scalar constants of type `float16'. This is a change from the behavior from before this PR.

Cause

The FuseOps pass gets the link_params flag from the Executor attribute from the IRModule, instead of taking it from the current target. If the current target has link_params=False, while the executor has link_params=True, this will lead to unexpected behavior for the current target.
In particular, when compiling a model that uses float16 constants, having link_params=True in FuseOps will prevent it from extracting these constants into parameters (which was the original behavior). This will then lead to more relay code being presented with ConstantNodes with type float16, which that code may not yet handle (as is in the case of ScheduleBuilder).

How it happens

Hexagon target sets link_params=True, while CPU target has link_params=False. During compilation, relay optimizations execute pass FoldConstants. This pass runs relay interpreter with a CPU target, regardless of the original compilation target. During the interpreter execution, FuseOps pass is executed. At this point the current target is CPU, while the IRModule's Executor settings correspond to the original "hexagon" target.

Edit: clarifications

@manupak
Copy link
Contributor

manupak commented Feb 25, 2022

Thanks @kparzysz-quic ! this makes more sense!

FoldConstants creates its own PassContext (that explains how the override is ignored) and Target but I do wonder why it needs just take the Executor ?

cc: @d-smirnov @Mousius

@manupak
Copy link
Contributor

manupak commented Feb 25, 2022

In the spirit of creating own config in the FoldConstants, could we just add the expected Executor config too ?

With<transform::PassContext> fresh_build_ctx(transform::PassContext::Create());
Map<String, ObjectRef> dict =
(module_->attrs.defined()) ? module_->attrs->dict : Map<String, ObjectRef>();

@kparzysz-quic
Copy link
Contributor Author

I think that should work...

@u99127
Copy link
Contributor

u99127 commented Feb 28, 2022

The relevant reproducer is here. #8509 (comment)

d-smirnov added a commit to d-smirnov/incubator-tvm that referenced this issue Mar 3, 2022
…link-params=0

Addressed issue apache#10390

Change-Id: I1a6b2dd27845f9292f1e07f9da1b9be722481f46
@d-smirnov
Copy link
Contributor

Fix: #10465

d-smirnov added a commit to d-smirnov/incubator-tvm that referenced this issue Mar 9, 2022
…link-params=0

Addressed issue apache#10390

Change-Id: I1a6b2dd27845f9292f1e07f9da1b9be722481f46
d-smirnov added a commit to d-smirnov/incubator-tvm that referenced this issue Mar 10, 2022
…link-params=0

Addressed issue apache#10390

Change-Id: I1a6b2dd27845f9292f1e07f9da1b9be722481f46
kparzysz-quic pushed a commit that referenced this issue Mar 10, 2022
…link-params=0 (#10465)

Addressed issue #10390

Change-Id: I1a6b2dd27845f9292f1e07f9da1b9be722481f46
@kparzysz-quic
Copy link
Contributor Author

This fixes the problem.

pfk-beta pushed a commit to pfk-beta/tvm that referenced this issue Apr 11, 2022
…link-params=0 (apache#10465)

Addressed issue apache#10390

Change-Id: I1a6b2dd27845f9292f1e07f9da1b9be722481f46
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants