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

[VTA][Runtime] fix hardcoded VerifyDep step #5471

Closed

Conversation

zhanghaohit
Copy link
Contributor

Bug Description

VerifyDep hardcode step check of 2, which is not true in other dependence settings (e.g., if we change the acc dependence distance in VTA hardware implementation)

Solution

add ACC_DEP_DISTANCE in vta_config.json and use that to derive checking step.

ACC_DEP_DISTANCE is also used in VTA hardware implementation when defining acc dependence distance (see here)

@zhanghaohit
Copy link
Contributor Author

@tqchen @tmoreau89 Could you kindly help review this PR? Thanks.

Copy link
Contributor

@tmoreau89 tmoreau89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @zhanghaohit for the proposed change, moving away from a hardcoded value is the right move!

I find the ACC_DEP_DISTANCE to be confusing however. Ultimately it's derived from the latency (in cycles) it takes for a write to address X to be visible on the read port at address X. Maybe we can dub this SRAM_WRITE_TO_READ_LATENCY or something along those lines?

@zhanghaohit
Copy link
Contributor Author

Thanks @zhanghaohit for the proposed change, moving away from a hardcoded value is the right move!

I find the ACC_DEP_DISTANCE to be confusing however. Ultimately it's derived from the latency (in cycles) it takes for a write to address X to be visible on the read port at address X. Maybe we can dub this SRAM_WRITE_TO_READ_LATENCY or something along those lines?

Thanks @tmoreau89 for the suggestion. I think VerifyDep here is to verify that we don't write to the same acc_mem index two cycles in a row (I may also need to change the comments).

The ACC_DEP_DISTANCE is the same as here. Did I get the correct meaning of VerifyDep?

@tqchen tqchen added the status: need update need update based on feedbacks label May 7, 2020
@tqchen
Copy link
Member

tqchen commented May 15, 2020

ping @tmoreau89 @zhanghaohit please followup :)

@zhanghaohit
Copy link
Contributor Author

zhanghaohit commented May 17, 2020

ping @tmoreau89 @zhanghaohit please followup :)

Thanks @tqchen for the reminder. I think this PR depends on this PR in VTA repo.
@tmoreau89 could you kindly review and what do you suggest? Thanks.

@tqchen tqchen closed this Oct 11, 2020
@zhanghaohit zhanghaohit deleted the bugfix/zhanghao/verifydep branch December 17, 2020 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: need update need update based on feedbacks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants