[TIR] Implement BufferPointer node #9391
Conversation
Lunderberg
requested review from
Hzfengsy,
ZihengJiang,
areusch,
comaniac,
jcf94,
jroesch,
junrushao,
kparzysz-quic,
masahi,
merrymercy,
tqchen,
vinx13,
yzhliu and
zhiics
as code owners
Lunderberg
force-pushed
the
buffer_pointer_node
branch
from
2a9bc38 to
517408d
Compare
|
Rebase on main to resolve conflict. |
Many lowering/optimization passes that interact with buffers have duplicated logic for handling the indices of the load/store. By having a separate BufferPointer node that is used to represent a pointer to a location inside a buffer, the common index handling can be de-duplicated. This is the primary change of this PR, with all subsequent commits being either changes required for or enabled by this change.
With BufferPointer now shared by both BufferLoad and BufferStore, updating the visitors so they follow the new graph.
In this commit, avoid any refactoring to make use of the new feature.
Previously, BufferStore could have value that was incomaptible with the buffer in which it was stored, which wasn't checked until the BufferStore node was lowered to Store nodes. As a result, unit tests that validate a single transformation could have incorrect types stored. Now, BufferStore checks that the value being stored matches the expected value type during its constructor, and so those unit tests require updates.
Previously, transforms that replace a buffer or modify the indices of a buffer needed to perform very similar replacements for both BufferLoad and BufferStore. Now that both of these operate on a BufferPointer, these passes can be simplified.
Lunderberg
force-pushed
the
buffer_pointer_node
branch
from
517408d to
6ac9275
Compare
jroesch
left a comment
jroesch
left a comment
There was a problem hiding this comment.
Conceptually makes sense to me.
|
We should have more discussions on RFC before merging this |
vinx13
left a comment
vinx13
left a comment
There was a problem hiding this comment.
Given it is common that only buffer + indices matters in rewriting, I'd suggest refactor into RewriteBufferAccess(pair<buffer, indices>) without modifying IR itself wherever possible. We can use a pair or auxiliary data structure BufferAccess (avoid potential confusion with concrete buffer pointer) to hold the input/output. We can discuss more in the RFC
|
@tqchen @jroesch @manupa-arm i think there were some concerns about escape analysis if pointer semantics are introduced to TIR. I think this has to do with USMP's inability to detect liveness by refcounting. i didn't take a deep look at the impl here, but could we resolve whether this is a problem before merging here? |
|
on second look, it looks like we're not adding a new TIR datatype which is a pointer, so nevermind about my previous concern about liveness analysis. |
|
Closing following discussion on RFC#0042. |
5 participants
This is intended to simplify transformations that impact BufferStore and BufferLoad nodes. As this impacts the TIR definitions, an RFC will be posted shortly.
This PR is divided into separate commits to help in review. The first commit is the main change adding an implementation of
BufferPointer, and modifyingBufferStoreandBufferLoad. The majority of the commits that follow are responses to this change, fixing breakages caused by the main change. The final commit "Updated transforms..." contains the simplifications to existing transformations that are enabled by the newBufferPointer.Edit: RFC posted here.