You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tvm.apache.org by Wuwei Lin <no...@github.com.INVALID> on 2022/05/10 23:25:54 UTC

[apache/tvm-rfcs] [RFC] Introducing DeclBuffer (PR #70)

This is a follow-up of https://github.com/apache/tvm/pull/9727 and [RFC#63](https://github.com/apache/tvm-rfcs/pull/63). Currently buffer can be implicitly declared and then used. The implicit behavior can be error prone and makes analysis more difficult. This RFC introduces `DeclBuffer`, a new IR construct as an explicit statement for buffer declaration.

Rendered version: https://github.com/vinx13/tvm-rfcs/blob/decl-buffer/rfcs/0070-introducing-decl-buffer.md

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

You can view, comment on, or merge this pull request online at:

  https://github.com/apache/tvm-rfcs/pull/70

-- Commit Summary --

  * [RFC] Introducing DeclBuffer

-- File Changes --

    A rfcs/0070-introducing-decl-buffer.md (210)

-- Patch Links --

https://github.com/apache/tvm-rfcs/pull/70.patch
https://github.com/apache/tvm-rfcs/pull/70.diff

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/70
You are receiving this because you are subscribed to this thread.

Message ID: &lt;apache/tvm-rfcs/pull/70@github.com&gt;

Re: [apache/tvm-rfcs] [RFC] Introducing DeclBuffer (PR #70)

Posted by Tianqi Chen <no...@github.com.INVALID>.
We always need the original syntax to enable bidirectional property. It does not hurt though to enable sugars that allows us to have a combination on top. How about we start with the original syntax and then discuss sugar as a separate topic

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/70#issuecomment-1149818365
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Introducing DeclBuffer (PR #70)

Posted by Wuwei Lin <no...@github.com.INVALID>.
@wrongtest Thanks for bringing up this. There are a few options for the behavior in TVM script, I'm open to discussion.

* A1: The original behavior before https://github.com/apache/tvm/pull/9727:
`T.allocate` returns a `Var`, which can be later used in `T.load / T.store`.

* A2: Current behavior: 
`T.allocate` returns a buffer (`Var` is implicitly created inside). The buffer can be accessed via subscripts, which will be translated to `BufferLoad / BufferStore`.

* A3: Potential behavior for this RFC:
`T.allocate` returns a `Var`, use `T.decl_buffer` to create buffer and access it (because `T.load / T.store` are deprecated). This follows the translation to TIR nodes closely.

* A4: Potential behavior for this RFC (the one @wrongtest is proposing):
If there are no buffer aliases (in most of the cases), it is tempting to avoid boilerplate code of `T.decl_buffer`. 
`T.allocate` returns a buffer, where `buffer->data` is implicitly created and allocated inside. The buffer can be accessed via subscripts. Subsequent buffer aliases can be created by referring to `buffer->data`. This requires `DeclBuffer` to be created implicitly, it is okay to have some discrepancy between parser side and the TIR nodes.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/70#issuecomment-1124141811
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Introducing DeclBuffer (PR #70)

Posted by wrongtest <no...@github.com.INVALID>.
Thanks a lot!  I think then we can handle buffer related issues in customized passes with more explicit and robust way.

I have one question on tir script, for certain algorithms in DL workloads, users may want to write non-stir formed script like
  ```python
  x = T.allocate((), "int32", "")
  x[()] = 0
  while x[()] < 128:
      x[()] = x[()] + 1
      # ...
  ```
   Could the parser support still write things like that (though underlying IR structure changed) instead of
   ```python
   x_data = T.allocate((), "int32", "")
   x = T.decl_buffer(data=x_data,)
   x[()] = 0
   # ...
   ```

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/70#issuecomment-1123341991
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Introducing DeclBuffer (PR #70)

Posted by wrongtest <no...@github.com.INVALID>.
reuse T.alloc_buffer seems good，as long as there is no ambiguity for parser impl ：）

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/70#issuecomment-1149935097
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Introducing DeclBuffer (PR #70)

Posted by Wuwei Lin <no...@github.com.INVALID>.
Seems we all agree that introducing `DeclBuffer` is helpful. The only unresolved question is how shall the TVMScript be updated as @wrongtest mentioned. As discussed above, we have the options:

* B1: In TVMScript, `T.allocate` and `T.decl_buffer` strictly map to the corresponding TIR nodes. To allocate and declare a buffer (in lowered TIR), there will be two separate steps:
```
data = T.allocate(physical_shape)
buffer = T.decl_buffer(data=data, shape=physical_shape)
```

* B2: provide some syntax sugar for `T.allocate` (adding a `returns_buffer` option), which is translated to `AllocateNode` + `DeclBufferNode` in TIR.
```
buffer = T.allocate(physical_shape, returns_buffer=True)
```
When printing them from TIR to TVMScript, we still print in unsugared form in B1.

Note that in B2, it is not feasible to make `T.allocate` always return the created buffer, because
1) there might be still need to directly use the buffer var in lower level TIR
2) when printing the TIR to TVMScript, `AllocateNode` and `DeclBufferNode` do not always appear in the same place, it is difficult (and not preferred) to map two TIR nodes into one TVMScript statement

would love to know what you think @wrongtest @Hzfengsy 

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/70#issuecomment-1149308146
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Introducing DeclBuffer (PR #70)

Posted by Wuwei Lin <no...@github.com.INVALID>.
@areusch @Hzfengsy I've updated the RFC. It is ready for another look

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/70#issuecomment-1150342730
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Introducing DeclBuffer (PR #70)

Posted by Andrew Reusch <no...@github.com.INVALID>.
Merged #70 into main.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/70#event-6781982914
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Introducing DeclBuffer (PR #70)

Posted by Wuwei Lin <no...@github.com.INVALID>.
@wrongtest I've thought about the option A3 vs A4. From the parsing / translation from TVM script to TIR, it is acceptable to have `T.allocate` translated to `Allocate + DeclBuffer` two nodes. But it will be tricky for `TVMScriptPrinter`. We will need to find both `Allocate` and `DeclBuffer` nodes and then print `T.allocate`, and these two nodes do not have to be parent/child of each other. I'm not sure if this behavior, which breaks 1-to-1 mapping between TVM script and TIR, is desirable. Alternatively, we can add an option to `T.allocate`, such as `def allocate(..., return_buffer: bool)`. What do you think?

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/70#issuecomment-1133153361
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>

Re: [apache/tvm-rfcs] [RFC] Introducing DeclBuffer (PR #70)

Posted by Tianqi Chen <no...@github.com.INVALID>.
A gentle ping. Would be great to have a status checkin and let us move to make this happen 

-- 
Reply to this email directly or view it on GitHub:
https://github.com/apache/tvm-rfcs/pull/70#issuecomment-1148570197
You are receiving this because you are subscribed to this thread.

Message ID: <ap...@github.com>