Lunderberg commented on PR #16569:
URL: https://github.com/apache/tvm/pull/16569#issuecomment-1994301290
Taking a look at that commit, there's a couple of things that stand out to
me.
* Related to `MergeSharedMemoryAllocations`
* The `MergeSharedMemoryAllocations` is a copy/paste of `StorageRewrite`
with modifications. The `StorageRewrite` pass is applied earlier and already
handled merging of shared memory allocations, so I'm not sure why the separate
pass is needed.
* It looks like there's some different descriptions of `StorageRewrite`.
[This 2021
comment](https://github.com/apache/tvm/pull/9341#issuecomment-949269726) states
that `StorageRewrite` cannot merge allocations of different dtypes. However,
[this 2018 PR](https://github.com/apache/tvm/pull/9341#issuecomment-949269726)
added memory-sharing of allocations of different dtypes for `StorageRewrite`.
This may be related to static vs dynamic shapes.
* The `StorageRewrite` pass only merged allocations that were within the
same `attr::thread_extent` annotation, and would be part of the same kernel.
Assuming that `MergeSharedMemoryAllocations` kept this behavior from
`StorageRewrite`, moving it after `SplitHostDevice` shouldn't be necessary.
* I'd have to dig into it to be sure, but using
`MergeSharedMemoryAllocations` after `ThreadSync("warp")` seems like it could
cause an issue. Applying `MergeSharedMemoryAllocations` (merge two buffers
into one) feels like it would undo the benefit of `ThreadSync("warp")`
(synchronize to avoid contention of a single buffer).
* Related to `HoistIfThenElse`
* The comment about requiring `HoistIfThenElse` to be before `UnrollLoop`
doesn't make sense to me. Any condition that could be statically proven for
all indices prior to `UnrollLoop` could also be statically proven for a
specific loop index. The end result of `UnrollLoop` and `HoistIfThenElse`
should be the same regardless of the ordering.
* Slight caveat: Unless the `HoistIfThenElse` causes a different
decision to be made by the automatic unroll heuristics in `UnrollLoop`.
However, those are disabled by default, and are not enabled by the unit test.
* Moving `HoistIfThenElse` changed it's ordering relative to
user-specified passes in the `"tir.add_lower_pass"` configuration. While I
don't know of any usage of this functionality, it's a bit worrying that it
occurred without discussion.
* The new function attribute `"tir.HoistIfThenElseExprWithBlock"`
overrides the user's configuration for `"tir.HoistIfThenElse"`. If the user
had selected a stronger hoisting to be performed (e.g. hoisting
`tir::IfThenElse` statements), it would no longer be applied.
* The configuration used when the new function attribute
`"tir.HoistIfThenElseExprWithBlock"` is set doesn't make sense for this pass's
location in the lowering flow. The `HoistIfThenElse` pass occurs after
`ConvertBlocksToOpaque` has removed all block variables, so the
`HoistedConditionals::kUsingBlockVar` flag shouldn't be required.
* Related to unit tests
* The unit tests added in `test_gpu_low_batch_gemv.py` only exercise the
meta-schedule functionality, and do not exercise any of the changes made to
`driver_api.cc` or to `HoistIfThenElse`.
--
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-unsubscr...@tvm.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
