RFC: clarifying buffer declaration and access by vinx13 · Pull Request #63 · apache/tvm-rfcs

vinx13 · 2022-03-18T22:52:45Z

In apache/tvm#9727 and RFC#39, we deprecated Load and Store to use BufferLoad and BufferStore instead in order to support generalized multi-dimensional physical buffer access. This is a follow-up that documents necessary clarifications, implications about the new buffer convention, as well as the post-hoc pass checklist.

Rendered version: https://github.com/vinx13/tvm-rfcs/blob/clarify-buffer-access/rfcs/0063-clarifying-buffer-declaration-and-access.md

cc @tqchen @Lunderberg @junrushao1994 @csullivan @mbs-octoml @jroesch @areusch @wrongtest @Hzfengsy

Co-authored-by: Eric Lunderberg <Lunderberg@users.noreply.github.com>

Lunderberg · 2022-04-08T16:11:28Z

+convention of `PrimFunc` and are translated to assertions of buffer shapes, strides, etc. in
+runtime.
+
+During the lowering process, although buffer accesses inside `PrimFunc` are flattened to match


I have a proof-of-concept implementation of this removal at apache/tvm#10940. It is able to pass all tests in test_target_codegen_llvm.py, and I don't see any roadblocks for updating the other tests as well.

Nice. We can then consider moving to explicit DeclBuffer (mentioned in this RFC) to define the alias.

areusch

thanks @vinx13 @Lunderberg for this clarifying RFC! did a fine-grained pass and attempted to clarify some of the wording per my understanding.

Co-authored-by: Andrew Reusch <areusch@gmail.com>

vinx13

@areusch Thanks for the great feedback, will update the RFC accordingly

vinx13 · 2022-04-08T20:23:42Z

+- Buffer map of `PrimFunc`. This specifies how opaque parameters of type `T.handle` should be
+interpreted as a buffer inside the `PrimFunc`. `T.handle` represents an opaque pointer (`void *`).
+- `T.alloc_buffer` is used `S-TIR` to create and allocate a buffer.
+- `T.buffer_decl` can be used to create a buffer alias by specifying the underlying data variable to


Yes. In TVM script, it is T.buffer_decl and we have plan to rename it to T.decl_buffer

vinx13 · 2022-04-08T20:27:58Z

+that has a vectorized type, or by using `Ramp` as an index into a buffer that has a scalar type.
+For N-D buffer indices, it is possible that `Ramp` being used in multiple dimensions
+(e.g. `A[Ramp(...), ..., Ramp(...)]` ). In this case the number of lanes of the data type of such
+value is the product of each `Ramp`. We may want to limit `Ramp` to only the last dimension as


@Lunderberg has already sent follow up PR to limit Ramp to the last dimension. I'll update this sentence

vinx13 · 2022-04-08T20:30:55Z

+for TIR transformations. Passes that rewrite buffers should clearly indicate how aliased buffers
+are handled. For example, when changing the underlying layout of stored elements in a buffer, all
+buffer aliases must also be updated. While buffer aliasing is typically free at runtime, this
+imposes a cost for buffer aliasing both to compile times and development complexity. 


we don't have any alias analysis right now

areusch · 2022-04-08T22:14:06Z

+- Buffer map of `PrimFunc`. This specifies how opaque parameters of type `T.handle` should be
+interpreted as a buffer inside the `PrimFunc`. `T.handle` represents an opaque pointer (`void *`).
+- `T.alloc_buffer` is used `S-TIR` to create and allocate a buffer.
+- `T.buffer_decl` can be used to create a buffer alias by specifying the underlying data variable to


ok, if you're using T. notation here, you mean TVMScript, right? in which case, can you update all occurrences to match what you would grep for if using TVMScript? right now it's a dead-end.

areusch · 2022-04-08T22:15:35Z

+
+`T.buffer_decl` creates a buffer alias if the underlying data variable (`.data` field) overlaps with
+another buffer. Buffer created via `alloc_buffer` always do not alias. Buffer aliases do not need
+`Allocate` to create the data variable. Each data variable can be allocated via `Allocate` once.


ok, can you update this in the doc? you could simply state "Buffer aliases do not need Allocate to create the data variable--they may simply reuse the data variable from the Buffer being aliased."

areusch · 2022-04-08T22:16:04Z

 We would like to discuss some examples of when it is safe to transform a buffer w.r.t. aliasing rules:
-
-(1) reshape, (2) layout transform (e.g. swap indices), (3) compact. 
+- (1) reshape


i think proper numbered list syntax is like
1. reshape 2. layout transform ... 3. compact

areusch · 2022-04-08T22:16:56Z

+
+(1) reshape, (2) layout transform (e.g. swap indices), (3) compact. 
+
+(1) is fine under aliasing as long as the low level memory is shared. (2) and (3) would need more


yeah that makes sense, can you update the RFC?

areusch · 2022-04-19T23:52:59Z

@vinx13 let me know when this is ready for another look!

vinx13 · 2022-04-20T18:35:26Z

@areusch it is ready for another look

areusch

thanks @vinx13, just a few things to ping on but otherwise no need to block on me.

areusch · 2022-04-29T19:20:47Z

+- Buffer map of `PrimFunc`. This specifies how opaque parameters of type `T.handle` should be
+interpreted as a buffer inside the `PrimFunc`. `T.handle` represents an opaque pointer (`void *`).
+- `T.alloc_buffer` is used `S-TIR` to create and allocate a buffer.
+- `T.buffer_decl` can be used to create a buffer alias by specifying the underlying data variable to


could you add a note so someone knows what to grep for post-future rename?

areusch · 2022-04-29T19:21:09Z

+
+**Explicit `DeclBuffer` IR construct**
+
+`T.buffer_decl` is not an explicit statement in TIR - There is no such node in TIR.


ping on this one?

areusch · 2022-04-29T19:26:44Z

+for TIR transformations. Passes that rewrite buffers should clearly indicate how aliased buffers
+are handled. For example, when changing the underlying layout of stored elements in a buffer, all
+buffer aliases must also be updated. While buffer aliasing is typically free at runtime, this
+imposes a cost for buffer aliasing both to compile times and development complexity. 


ok, perhaps we could state that in RFC i guess? it seems pretty useful to have that if we make this change.

vinx13 · 2022-04-29T19:38:44Z

@areusch added some minor updates based on the comments, see the last two commits

areusch

thanks for the updates and sorry for the long delay!

vinx13 mentioned this pull request Mar 18, 2022
[Tracking Issue] BufferAccess Migration Followup Items apache/tvm#10505
Closed
19 tasks

RFC: clarifying buffer declaration and access
116cdc4
Co-authored-by: Eric Lunderberg <Lunderberg@users.noreply.github.com>

vinx13 force-pushed the clarify-buffer-access branch from 507c133 to 116cdc4Compare March 20, 2022 07:06

Lunderberg reviewed Apr 8, 2022
View reviewed changes

areusch reviewed Apr 8, 2022
View reviewed changes

Apply suggestions from code review
8c4ebb2
Co-authored-by: Andrew Reusch <areusch@gmail.com>

vinx13 commented Apr 8, 2022
View reviewed changes

vinx13 added 4 commits April 8, 2022 13:55

address comments
02d6c79

address comments
3d7c0fb

address comments
ad42e71

address comments
e8c278b

areusch reviewed Apr 8, 2022
View reviewed changes

Address comments
12e793e

add note about renaming to T.decl_buffer
c108f91

areusch approved these changes Apr 29, 2022
View reviewed changes

Update 0063-clarifying-buffer-declaration-and-access.md
5dbe413

vinx13 mentioned this pull request May 10, 2022
[RFC] Introducing DeclBuffer #70
Merged

areusch approved these changes May 11, 2022
View reviewed changes

areusch merged commit de4fe97 into apache:mainMay 11, 2022


		(1) reshape, (2) layout transform (e.g. swap indices), (3) compact.

		(1) is fine under aliasing as long as the low level memory is shared. (2) and (3) would need more


		Explicit `DeclBuffer` IR construct

		`T.buffer_decl` is not an explicit statement in TIR - There is no such node in TIR.

Uh oh!

Conversation

vinx13 commented Mar 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

areusch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vinx13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

areusch commented Apr 19, 2022

Uh oh!

vinx13 commented Apr 20, 2022

Uh oh!

areusch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vinx13 commented Apr 29, 2022

Uh oh!

areusch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vinx13 commented Mar 18, 2022•
edited
Loading