[TIR] Remove PrimFuncNode::preflattened_buffer_map #10940
Conversation
|
|
Lunderberg
force-pushed
the
remove_preflattened_buffer_map
branch
from
4e7ec70 to
7062789
Compare
`PrimFuncNode::preflattened_buffer_map` was introduced in apache#9727, in order to maintain a record of the pre-flattened buffer shape until it can be used in `MakePackedAPI`. This commit instead maintains the pre-flattened shapes in `PrimFuncNode::buffer_map`, while the body of the function uses a flattened buffer alias. Passes LLVM tests in test_target_codegen_llvm.py as initial proof of concept.
Lunderberg
force-pushed
the
remove_preflattened_buffer_map
branch
from
e6c5d51 to
877cc24
Compare
|
Rebased to resolve merge conflict, expecting same breakage in the ethos-u unit tests. |
This was incorrectly done in the first pass through, caught by CI.
areusch
added
needs-triage
* Remove additional preflatten usage * Use tuple instead of list for `T.Buffer`, as lists now throw * A couple numeric updates
There were a couple of spots that needed to be aware of flattened buffers being represented as buffer aliases.
Forwarding the instrumentation when evaluating FoldConstants helped in debugging, but shouldn't have made it in.
|
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
changed the title
Previous parser contextually interpreted buffer subscripts as Ramp nodes if they occurred as part of an expression. For new parser, need to use an explicit step size to generate a Ramp.
`PrimFuncNode::preflattened_buffer_map` was introduced in apache#9727, in order to maintain a record of the pre-flattened buffer shape until it can be used in `MakePackedAPI`. This commit instead maintains the pre-flattened shapes in `PrimFuncNode::buffer_map`, while the body of the function uses a flattened buffer alias, as described in [RFC#70](apache/tvm-rfcs#70)
| return buf_map_[buffer.get()]; | ||
| } else { | ||
| LOG(FATAL) << "Can't work around the undefined buffer"; | ||
| return *static_cast<BufferEntry*>(nullptr); |
There was a problem hiding this comment.
This is not good, please use __builtin_unreachable() or something like that...
There was a problem hiding this comment.
Given that the effect of control flow reaching __builtin_unreachable() is to make the program behavior undefined, I had assumed that intentionally invoking undefined behavior in an unreachable location would be identical to __builtin_unreachable(), without relying on non-standard intrinsics. The C++23 implementation of std::unreachable() has the same semantics, that program behavior is undefined if the unreachable location is reached.
That said, this pattern is primarily to suppress the warning that occurs if a return value isn't in a branch, and isn't the most readable. Testing, it looks like a much better way to suppress the warning would be to mark LOG(FATAL) as [[noreturn]], so that the warning never needs to be suppressed in the first place.
There was a problem hiding this comment.
LOG(FATAL) should already be [[noreturn]], (this has come up before). Clang 14 warns about the "reference to null", so this actually generates a warning for me. No matter what, as it is here, this is an explicit UB.
There was a problem hiding this comment.
Looks like there were two independent LOG(FATAL) implementations, and only the one that occurred when TVM_LOG_CUSTOMIZE was defined has [[noreturn]] applied to it.
There was a problem hiding this comment.
No matter what, as it is here, this is an explicit UB.
Agreed. That was intentional as a way to indicate an unreachable path without compiler-specific intrinsics, though I'm realizing that my intent for it to be both (1) deducible by the compiler as undefined for the purpose of optimization and (2) not deducible by the compiler as undefined for the purpose of producing warnings wasn't the most consistent of plans.
There was a problem hiding this comment.
I've opened #13542, which removes this line entirely, with the [[noreturn]] attribute applied to both implementations of LOG(FATAL) so that the warning does not occur.
PrimFuncNode::preflattened_buffer_mapwas introduced in #9727, in order to maintain a record of the pre-flattened buffer shape until it can be used inMakePackedAPI. This commit instead maintains the pre-flattened shapes inPrimFuncNode::buffer_map, while the body of the function uses a flattened buffer alias.Passes LLVM tests in test_target_codegen_llvm.py as initial proof of concept.