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

[realy][pass]add split infer shape with convert op layout pass #11825

Merged
merged 1 commit into from
Jun 23, 2022
Merged

[realy][pass]add split infer shape with convert op layout pass #11825

merged 1 commit into from
Jun 23, 2022

Conversation

chengven027
Copy link
Contributor

add SplitInferCorrectLayout.

@chengven027
Copy link
Contributor Author

cc @masahi please review when you got time.

@masahi masahi merged commit a090009 into apache:main Jun 23, 2022
@chengven027 chengven027 deleted the split.add.infer branch June 24, 2022 07:44
blackkker pushed a commit to blackkker/tvm that referenced this pull request Jul 7, 2022
@masahi
Copy link
Member

masahi commented Aug 17, 2022

@chengven027-intellif I've just hit a bug with SplitInferCorrectLayout. The model I have does split op which splits along C dim in NCHW like this:

  %514 = multiply(%512, %513) /* ty=Tensor[(1, 256, 30, 17), float16] */;
  %515 = split(%514, indices_or_sections=[128], axis=-3) /* ty=(Tensor[(1, 128, 30, 17), float16], Tensor[(1, 128, 30, 17), float16]) */;

This is going to be a problem for x86 or other backend that uses NCHWc layout. Since the original splitting axis is blocked along two axes, SplitInferCorrectLayout needs to change both indices_or_sections and axis parameters. Right now our split op supports only integer axis, so this change could be difficult.

Unless you have a solution, I think we need to revert this PR. Or specially handle NCHWc layout by giving up layout fall-through (insert layout_transform) if the input layout is NCHWc.

You can reproduce this error by running https://gist.github.com/masahi/dce79525a5d0bd9c8a5d04992c060ab0

@chengven027
Copy link
Contributor Author

ok, thanks. Maybe I didn't consider this situation.

@alter-xp
Copy link
Contributor

This is going to be a problem for x86 or other backend that uses NCHWc layout.

hi @chengven027-intellif @masahi, The same problem occurred to my model. So in addition to disabling this part of the code, is there a good solution to this?

@masahi
Copy link
Member

masahi commented Sep 20, 2022

There is no easy solution, we should revert this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants