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

[ARM] Fix concat #3061

Merged
merged 1 commit into from
May 17, 2019
Merged

[ARM] Fix concat #3061

merged 1 commit into from
May 17, 2019

Conversation

hlu1
Copy link
Contributor

@hlu1 hlu1 commented Apr 21, 2019

Fix for https://discuss.tvm.ai/t/relay-build-target-rasp3b-something-wrong/2195
I added arm_cpu schedule for concat with no vectorization.New test case is also added to cover the special case that triggered this bug.

@FrozenGene, please review

@FrozenGene
Copy link
Member

I still meet the same error. @hlu1

v0.0.1
%3 = fn (%p0: Tensor[(3, 256), float32], %p1: Tensor[(3, 256), float32], dict=meta[StrMap][0]) -> Tensor[(3, 512), float32] {
%0 = (%p0, %p1)
%1 = concatenate(%0, axis=1) // ty=Tensor[(3, 512), float32]
%2 = nn.batch_flatten(%1) // ty=Tensor[(3, 512), float32]
%2
}
%3

@hlu1
Copy link
Contributor Author

hlu1 commented Apr 23, 2019

The root cause of this problem is that the way concat implemented is not vectorizable. It uses tvm::if_then_else (https://github.com/dmlc/tvm/blob/master/topi/include/topi/transform.h#L338), which cannot be vectorized. Writing a separate schedule for concat doesn't completely solve the problem because concat is marked as injective op and once it's fused with other injective ops, the fused_op is scheduled with the injective schedule instead of the concat schedule. One temporary solution is to remove vectorization from the injective schedule, which is not optimal. Another solution is to fix the if_then_else problem, which isn't quite obvious to me.

@FrozenGene
Copy link
Member

FrozenGene commented Apr 24, 2019

@tqchen tvm::if_then_else I remember is introduced by one commit of you, right? Could you help to see the problem of @hlu1 mentioned?

@tqchen
Copy link
Member

tqchen commented Apr 25, 2019

I think we could use some discussion here. if_then_else is a "safe" version of select when we cannot simply read out values from both sides and there are some branches involved.

Although for certain systems, OOB read is fine and select could just be used. Ideally, we would like to have a detector that detects if vectorization can be applied, and only apply it when it finds that the compute-pattern is OK. We could move some of the discussion to a new RFC.

@hlu1 @FrozenGene please share your thoughts

@tqchen tqchen added the status: need update need update based on feedbacks label Apr 25, 2019
@tqchen
Copy link
Member

tqchen commented Apr 27, 2019

ping @hlu1

@tqchen tqchen merged commit 78a0f47 into apache:master May 17, 2019
@tqchen
Copy link
Member

tqchen commented May 17, 2019

Merge this in for now as it is a strict improvement over previous ones

@tqchen tqchen added status: accepted and removed status: need update need update based on feedbacks labels May 17, 2019
@tqchen
Copy link
Member

tqchen commented May 17, 2019

Thanks, @hlu1 @FrozenGene

hlu1 added a commit to hlu1/tvm that referenced this pull request May 23, 2019
@hlu1 hlu1 deleted the arm_concatenate branch May 31, 2019 21:19
wweic pushed a commit to wweic/tvm that referenced this pull request Jun 26, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants