You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/09/23 19:58:44 UTC
[GitHub] [tvm] Lunderberg opened a new pull request, #12891: [TIR][Transform] Clear buffer_map during MakeUnpackedAPI
Lunderberg opened a new pull request, #12891:
URL: https://github.com/apache/tvm/pull/12891
This mimics the behavior in `MakePackedAPI`, and is assumed to be the case for some codegens.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #12891: [TIR][Transform] Clear buffer_map during MakeUnpackedAPI
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #12891:
URL: https://github.com/apache/tvm/pull/12891#issuecomment-1258215543
@Mousius Thank you, and it looks like that's also being caught in the tests, as this breaks pretty much every ethos-u test at [this line](https://github.com/apache/tvm/blob/main/python/tvm/relay/backend/contrib/ethosu/tir_to_cs_translator.py#L265). (Also why I intentionally marked this as a draft PR, to catch any impact to workflows that I'm not as familiar with.)
To me, the `buffer_map` is an indicator for how a function should later be lowered, and what buffers should be defined as part of the lowered function. Once it has been used, then it makes sense for the resulting function to have an empty `buffer_map`, because no further lowering is required. After #9727, the buffer shape/size could still be accessed by later passes find finding the `BufferLoad` or `BufferStore` object that makes use of a parameter, rather than searching in the `buffer_map` itself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #12891: [TIR][Transform] Clear buffer_map during MakeUnpackedAPI
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #12891:
URL: https://github.com/apache/tvm/pull/12891#issuecomment-1258300786
> I figured that was why it was in draft, being a bit nosey as I remember wondering about it when I was writing MakeUnpackedAPI :smile_cat:
No worries at all, and I wouldn't consider it nosey, either. It's the exact type of comment I was hoping for, as there wasn't an immediately obvious reason for the asymmetry between packed and unpacked.
> Also can we add to the documentation of the Pass the intention behind clearing it?
Good call, and will do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius merged pull request #12891: [TIR][Transform] Clear buffer_map during MakeUnpackedAPI
Posted by GitBox <gi...@apache.org>.
Mousius merged PR #12891:
URL: https://github.com/apache/tvm/pull/12891
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #12891: [TIR][Transform] Clear buffer_map during MakeUnpackedAPI
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #12891:
URL: https://github.com/apache/tvm/pull/12891#issuecomment-1258298073
> The fix makes sense to me, can we either make use of [get_loads](https://github.com/apache/tvm/blob/497f5f62230935a15c1af6d528890f9b76317445/python/tvm/relay/backend/contrib/ethosu/tir/utils.py#L161) and [get_stores](https://github.com/apache/tvm/blob/497f5f62230935a15c1af6d528890f9b76317445/python/tvm/relay/backend/contrib/ethosu/tir/utils.py#L185) or move the collect_buffer_map function to utils.py?
Certainly. It is now pulled out and documented, with defined behavior for buffers that share a backing `Var`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on a diff in pull request #12891: [TIR][Transform] Clear buffer_map during MakeUnpackedAPI
Posted by GitBox <gi...@apache.org>.
Mousius commented on code in PR #12891:
URL: https://github.com/apache/tvm/pull/12891#discussion_r981109968
##########
python/tvm/relay/backend/contrib/ethosu/tir_to_cs_translator.py:
##########
@@ -254,15 +255,19 @@ def extract_param_base_addresses(mod, buffer_info, scratch_region_map) -> List[u
assert len(mod.functions.items()) == 1
primfunc = mod.functions.items()[0][1]
+ buffer_map = tir_utils.collect_buffer_map(primfunc.body)
+
base_addresses = list()
idx = 0
+
for param in primfunc.params:
# constants are pooled together and handled specially
# this will change after tir.allocate_const.
# For now, we are skipping generating buffer addresses here
if buffer_info[param].btype == BufferType.constant:
continue
- buffer = primfunc.buffer_map[param]
+
Review Comment:
I've just chatted with @ekalda around this and we're not sure why the extra buffer appears, it appears to be an unused intermediary buffer and for the tests I ran this fixes it:
```suggestion
if param not in buffer_map:
base_addresses.append(
util.BaseAddress(
param.name.replace("-", "_"),
idx,
_get_region(buffer_info[param].btype, param, scratch_region_map),
0,
)
)
idx += 1
continue
```
I can tidy this loop up afterwards :smile_cat:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on pull request #12891: [TIR][Transform] Clear buffer_map during MakeUnpackedAPI
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on PR #12891:
URL: https://github.com/apache/tvm/pull/12891#issuecomment-1258229042
@Mousius Can you check the latest commit on this PR? I didn't try to re-order the passes, but reading from the `BufferStore`/`BufferLoad` objects instead of the `buffer_map` resolved the test cases I ran locally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #12891: [TIR][Transform] Clear buffer_map during MakeUnpackedAPI
Posted by GitBox <gi...@apache.org>.
Mousius commented on PR #12891:
URL: https://github.com/apache/tvm/pull/12891#issuecomment-1260092524
Thanks for making the API that little bit more consistent @Lunderberg 😸
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #12891: [TIR][Transform] Clear buffer_map during MakeUnpackedAPI
Posted by GitBox <gi...@apache.org>.
Mousius commented on PR #12891:
URL: https://github.com/apache/tvm/pull/12891#issuecomment-1257728044
@Lunderberg I think we make use of this in following passes and I'm unsure why make packed API decides to clear it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #12891: [TIR][Transform] Clear buffer_map during MakeUnpackedAPI
Posted by GitBox <gi...@apache.org>.
Mousius commented on PR #12891:
URL: https://github.com/apache/tvm/pull/12891#issuecomment-1258232368
> To me, the `buffer_map` is an indicator for how a function should later be lowered, and what buffers should be defined as part of the lowered function. Once it has been used, then it makes sense for the resulting function to have an empty `buffer_map`, because no further lowering is required. After #9727, the buffer shape/size could still be accessed by later passes find finding the `BufferLoad` or `BufferStore` object that makes use of a parameter, rather than searching in the `buffer_map` itself, or the passes could be moved to an earlier stage of lowering.
Aha! Makes perfect sense, thanks for the explanation! I figured that was why it was in draft, being a bit nosey as I remember wondering about it when I was writing `MakePackedAPI` 😸
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Lunderberg commented on a diff in pull request #12891: [TIR][Transform] Clear buffer_map during MakeUnpackedAPI
Posted by GitBox <gi...@apache.org>.
Lunderberg commented on code in PR #12891:
URL: https://github.com/apache/tvm/pull/12891#discussion_r981396998
##########
python/tvm/relay/backend/contrib/ethosu/tir_to_cs_translator.py:
##########
@@ -254,15 +255,19 @@ def extract_param_base_addresses(mod, buffer_info, scratch_region_map) -> List[u
assert len(mod.functions.items()) == 1
primfunc = mod.functions.items()[0][1]
+ buffer_map = tir_utils.collect_buffer_map(primfunc.body)
+
base_addresses = list()
idx = 0
+
for param in primfunc.params:
# constants are pooled together and handled specially
# this will change after tir.allocate_const.
# For now, we are skipping generating buffer addresses here
if buffer_info[param].btype == BufferType.constant:
continue
- buffer = primfunc.buffer_map[param]
+
Review Comment:
That works for me locally, on to the CI! Thank you on the ethos-u debugging, and it helps quite a bit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org