-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Add check for non-contiguous memory access when lowering to async dma… #13613
Conversation
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.
Generated by tvm-bot |
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.
LGTM. Just one nit.
auto store_iter_map = DetectIterMap(store_index, input_iters, 1, arith::IterMapLevel::NoCheck, | ||
&analyzer, false); | ||
if (!store_iter_map->errors.empty()) { | ||
LOG(FATAL) << "Unable to lower async dma for non contiguous memory access with index: " |
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.
Nit: You might differentiate the error message by including something text indicating that the "buffer store" contained the non contiguous access.
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.
Added!
auto load_iter_map = | ||
DetectIterMap(load_index, input_iters, 1, arith::IterMapLevel::NoCheck, &analyzer, false); | ||
if (!load_iter_map->errors.empty()) { | ||
LOG(FATAL) << "Unable to lower async dma for non contiguous memory access with index: " |
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.
Nit: Same as above. Add text indicating "buffer load" contained non contiguous access.
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.
Added!
What support is being planed? 2D DMA or breaking up into contiguous copies? |
We are still discussing the pros and cons of the potential options but the possible solutions are.
|
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.
Need to fix the lint
apache#13613) * Add check for non-contiguous memory access when lowering to async dma copies. * lint * lint and nits * lint
apache#13613) * Add check for non-contiguous memory access when lowering to async dma copies. * lint * lint and nits * lint
… copies.
Currently we do not support non contiguous memory copies for async dma so this adds a check to error out in the case that either load or store is non contiguous. Unfortunately if we do not throw an error in this case then we get inaccurate outputs. Added a test to verify that we are indeed throwing an error in this case that should be removed once support is added.
@adstraw @masahi