Skip to content

Conversation

@Lunderberg
Copy link
Contributor

Prior to this commit, any use of tvm.nd.from_dlpack to create a strided NDArray, or a NDArray whose alignment was less than tvm::runtime::kAllocAlignment would raise an error. As a result, views into larger arrays, which are unlikely to be aligned and compact, could only be shared when copied into an aligned and compact buffer.

This commit moves the compact/aligned check from the NDArray class into the generated TIR code as part of DLTensor unpacking. These checks were initially introduced in #11391, to avoid segfaults caused by use of non-aligned buffers in code intended for aligned buffers. The new checks will provide the same safeguard as the alignment is checked prior to use, but allows the alignment requirement to be relaxed on a per-buffer basis.

This approach also removes a potential bug resulting from compile-time configuration of tvm::runtime::kAllocAlignment, first introduced in #13307. Since TVM supports cross-compiling, the installation of TVM used to compile a kernel may assume a larger value of kAllocAlignment than is provided by the runtime installation of TVM. By validating the alignment within the generated kernel, rather than as part of the runtime, this potential inconsistency would be caught.

@tvm-bot
Copy link
Collaborator

tvm-bot commented May 4, 2023

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

@Lunderberg Lunderberg force-pushed the move_alignment_compact_checks_to_tir branch from 617c5bc to 8c9cd3b Compare May 5, 2023 14:36
@Lunderberg
Copy link
Contributor Author

It looks like this commit breaks most of the Rust tests, as the alignment assumed by the LLVM kernel was never validated when running through Rust. Unfortunately, I can't find a good way to produce aligned allocations in Rust, as Vec::new_in is unstable. For now, I've updated the Rust tests to opt out of requiring aligned buffers, but this isn't the best long-term solution.

Lunderberg added a commit to Lunderberg/tvm that referenced this pull request May 5, 2023
This utility was useful during debugging that resulted in the changes
made in apache#14771, but is not
otherwise related to that PR.
junrushao pushed a commit that referenced this pull request May 6, 2023
This utility was useful during debugging that resulted in the changes
made in #14771, but is not
otherwise related to that PR.
@Lunderberg Lunderberg force-pushed the move_alignment_compact_checks_to_tir branch from 0a43269 to db500b8 Compare May 8, 2023 17:26
@Lunderberg
Copy link
Contributor Author

Lunderberg commented May 8, 2023

@areusch Could you take a look at the AOT change made in the most recent commit on this PR (link)? By changing the LLVM assumption to an assert, it's shaking out a number of locations where the assumption didn't hold, but I'm not very familiar with AOT.

@Lunderberg Lunderberg force-pushed the move_alignment_compact_checks_to_tir branch from 4ba735d to f79bf72 Compare May 15, 2023 20:03
@Lunderberg Lunderberg force-pushed the move_alignment_compact_checks_to_tir branch from f8c1b78 to d9ea602 Compare May 16, 2023 21:22
Lunderberg added a commit to Lunderberg/tvm that referenced this pull request Jul 5, 2023
By default, the LLVM codegen is allowed to assume that
externally-allocated buffers are 64-byte aligned (from the value of
`kAllocAlignment`).  The buffers allocated in the Rust unit tests do
not provide this alignment, and need to specify that no additional
alignment is provided.

This is a subset of changes made in
apache#14771, broken out into an
independent PR for ease of testing/review.
Lunderberg added a commit to Lunderberg/tvm that referenced this pull request Jul 5, 2023
Prior to this commit, constants were aligned to 16-byte regions within
the allocation pool.  As a result, the pointer passed to packed
functions would usually not meet the default alignment requirement of
`kAllocAlignment = 64` bytes.  Because LLVM is allowed to assume that
this alignment is met, this could result in runtime errors.

This commit updates the default alignment to `kAllocAlignment`, which
can be overridden on a per-buffer basis.

This is a subset of changes made in
apache#14771, broken out into an
independent PR for ease of testing/review.
Lunderberg added a commit to Lunderberg/tvm that referenced this pull request Jul 5, 2023
The default TVM allocators provide allocations with 64-byte
alignment.  This alignment is provided as a guarantee to LLVM
optimizations, and failure to provide aligned allocations may result
in incorrect results.  This commit updates the MicroTVM examples to
provide 64-byte aligned memory allocations.

This is a subset of changes made in
apache#14771, broken out into an
independent PR for ease of testing/review.
Lunderberg added a commit to Lunderberg/tvm that referenced this pull request Jul 5, 2023
The default TVM allocators provide allocations with 64-byte
alignment.  This alignment is provided as a guarantee to LLVM
optimizations, and failure to provide aligned allocations may result
in incorrect results.  This commit updates the CRT examples to
provide 64-byte aligned memory allocations.

This is a subset of changes made in
apache#14771, broken out into an
independent PR for ease of testing/review.
@Lunderberg Lunderberg force-pushed the move_alignment_compact_checks_to_tir branch from 87dda27 to 8b5c779 Compare July 5, 2023 18:21
@Lunderberg
Copy link
Contributor Author

Rebased onto main to resolve conflict with #15132. The individual commits are now cleaned up and ordered such that, if CI failures continue to occur, they can be segmented out into independent PRs.

The `TVMAotExecutor_GetInputName` function requires a `const char**`,
but is provided with a `char**`.  While C's automatic pointer
conversion does allow this as an automatic conversion, following
const-correctness avoids a compile-time warning.

This warning was first caused following
apache#14529, which itself was to follow
const-correctness within the implementation of
`TVMAotExecutor_GetInputName`.
By default, the LLVM codegen is allowed to assume that
externally-allocated buffers are 64-byte aligned (from the value of
`kAllocAlignment`).  The buffers allocated in the Rust unit tests do
not provide this alignment, and need to specify that no additional
alignment is provided.

This is a subset of changes made in
apache#14771, broken out into an
independent PR for ease of testing/review.
Prior to this commit, constants were aligned to 16-byte regions within
the allocation pool.  As a result, the pointer passed to packed
functions would usually not meet the default alignment requirement of
`kAllocAlignment = 64` bytes.  Because LLVM is allowed to assume that
this alignment is met, this could result in runtime errors.

This commit updates the default alignment to `kAllocAlignment`, which
can be overridden on a per-buffer basis.

This is a subset of changes made in
apache#14771, broken out into an
independent PR for ease of testing/review.
The default TVM allocators provide allocations with 64-byte
alignment.  This alignment is provided as a guarantee to LLVM
optimizations, and failure to provide aligned allocations may result
in incorrect results.  This commit updates the MicroTVM examples to
provide 64-byte aligned memory allocations.

This is a subset of changes made in
apache#14771, broken out into an
independent PR for ease of testing/review.
The default TVM allocators provide allocations with 64-byte
alignment.  This alignment is provided as a guarantee to LLVM
optimizations, and failure to provide aligned allocations may result
in incorrect results.  This commit updates the CRT examples to
provide 64-byte aligned memory allocations.

This is a subset of changes made in
apache#14771, broken out into an
independent PR for ease of testing/review.
Prior to this commit, any use of `tvm.nd.from_dlpack` to create a
strided `NDArray`, or a `NDArray` whose alignment was less than
`tvm::runtime::kAllocAlignment` would raise an error.  As a result,
views into larger arrays, which are unlikely to be aligned and
compact, could only be shared when copied into an aligned and compact
buffer.

This commit moves the compact/aligned check from the `NDArray` class
into the generated TIR code as part of DLTensor unpacking.  These
checks were initially introduced in
apache#11391, to avoid segfaults caused by
use of non-aligned buffers in code intended for aligned buffers.  The
new checks will provide the same safeguard as the alignment is checked
prior to use, but allows the alignment requirement to be relaxed on a
per-buffer basis.

This approach also removes a potential bug resulting from compile-time
configuration of `tvm::runtime::kAllocAlignment`, first introduced in
apache#13307.  Since TVM supports
cross-compiling, the installation of TVM used to compile a kernel may
assume a larger value of `kAllocAlignment` than is provided by the
runtime installation of TVM.  By validating the alignment within the
generated kernel, rather than as part of the runtime, this potential
inconsistency would be caught.

This check is also restricted to targets whose `void*` opaque pointer
can be interpreted as a pointer to the data array.  (e.g. No such
check applies on Vulkan, as the `void*` is a pointer to a struct that
contains additional bookkeeping.)
@Lunderberg Lunderberg force-pushed the move_alignment_compact_checks_to_tir branch from 8b5c779 to b265574 Compare May 23, 2024 18:50
@Lunderberg
Copy link
Contributor Author

Rebased this onto main, as I attempt to clear out some of my (very) old PRs.

@Lunderberg
Copy link
Contributor Author

Closing this PR, as it is unlikely that I will have a chance to get back to it.

If anybody else would like to pick this up, the last roadblock was that the Rust allocator does not provide the higher alignment requirement that is later assumed that memory allocation, and doesn't have a clear way to provide the larger-than-element-type alignment.

@Lunderberg Lunderberg closed this Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants