Iteration over use lists in writeUseListOrders is non-deterministic as a
result of using a DenseMap. Replacing with a Vector-backed `MapVector`
restores deterministic behaviour.
When parseCustomEntry() calls a user attribute/type callback that
internally reads sub-attributes/types via the bytecode reader, the
reader may add entries to the deferredWorklist if the depth limit is
exceeded. If the callback then returns success with an empty entry
(falling through to the regular dialect reader), the reader position is
reset but deferredWorklist retains stale entries from the failed partial
read.
This causes an assert(deferredWorklist.empty()) failure in debug builds
when the fallback dialect reader successfully parses the attribute.
Fix by saving and restoring deferredWorklist.size() around each callback
invocation, discarding any stale entries added during a callback's
partial read when the reader position is rolled back.
Fixes#163337
Assisted-by: Claude Code
This adds a singular Builtin dialect version for use with bytecode
serialization. This version is not currently print unless set and not 0
(not planned a bump until next LLVM version). Created a unit test as
this was easiest way to track.
Additionally add emitWarning virtual method to DialectBytecodeReader,
mirroring emitError.
Tested on old mlir-opt reader, and could read, so should be non-breaking
change.
Extra cost is in serialization layer localized while resulting in
smaller bytecode files, this also keeps the format compatible with what
was previously.
The bytecode reader could enter an infinite loop when parsing deeply
nested attributes containing type references. The deferred worklist
stored only indices without distinguishing between attributes and types,
causing type indexes to be misinterpreted as attributes.
This patch changes the deferred worklist to store pairs of (index, kind)
to track whether each deferred entry is a type or attribute. The
worklist processing logic is updated to resolve the correct entry type.
Add ability to defer parsing and re-enqueueing oneself. This enables
changing CallSiteLoc parsing to not recurse as deeply: previously this
could fail (especially on large inputs in debug mode the recursion could
overflow). Add a default depth cutoff, this could be a parameter later
if needed.
Roll-forward of #170993 with relatively direct change such that if
processing while not resolving/when parsing property it eagerly
resolves.
uint64_t and size_t are not the same across all platforms. This was
causing build failures when building this file for wasm:
llvm-project/mlir/lib/Bytecode/Reader/BytecodeReader.cpp:1323:19: error:
out-of-line definition of 'resolveEntry' does not match any declaration
in '(anonymous namespace)::AttrTypeReader'
1323 | T AttrTypeReader::resolveEntry(SmallVectorImpl<Entry<T>>
&entries, size_t index,
| ^~~~~~~~~~~~
third_party/llvm/llvm-project/mlir/lib/Bytecode/Reader/BytecodeReader.cpp:851:7:
note: AttrTypeReader defined here
851 | class AttrTypeReader {
| ^~~~~~~~~~~~~~
1 error generated.
Use uint64_t everywhere to ensure portability.
Add ability to defer parsing and re-enqueueing oneself. This enables
changing CallSiteLoc parsing to not recurse as deeply: previously this
could fail (especially on large inputs in debug mode the recursion could
overflow). Add a default depth cutoff, this could be a parameter later
if needed.
Adds a check that the bytecode buffer is aligned to any section
alignment requirements. Without this check, if the source buffer is not
sufficiently aligned, we may return early when aligning the data
pointer. In that case, we may end up trying to read successive sections
from an incorrect offset, giving the appearance of invalid bytecode.
This requirement is documented in the bytecode unit tests, but is not
otherwise documented in the code or bytecode reference.
Fixes#150163
MLIR bytecode does not preserve alias definitions, so each attribute
encountered during deserialization is treated as a new one. This can
generate duplicate `DISubprogram` nodes during deserialization.
The patch adds a `StringMap` cache that records attributes and fetches
them when encountered again.
- try_emplace(Key) is shorter than insert({Key, nullptr}).
- try_emplace performs value initialization without value parameters.
- We overwrite values on successful insertion anyway.
We already have hasOneUse. Like llvm::Value we provide helper methods to
query the number of uses of a Value. Add unittests for Value, because
that was missing.
---------
Co-authored-by: Michael Maitland <michaelmaitland@meta.com>
I observed that we have the boundary comments in the codebase like:
```
//===----------------------------------------------------------------------===//
// ...
//===----------------------------------------------------------------------===//
```
I also observed that there are incomplete boundary comments. The
revision is generated by a script that completes the boundary comments.
```
//===----------------------------------------------------------------------===//
// ...
...
```
Signed-off-by: hanhanW <hanhan0912@gmail.com>
Update `BytecodeWriter` to invoke `reserveExtraSpace` on the stream
before writing to it. This will give clients implementing custom output
streams the opportunity to allocate an appropriately sized buffer for
the write.
The config is currently not movable and because there are constructors
the default move won't be generated, which prevents it from being moved.
Also, it is not copyable because of the unique_ptr. This PR adds move
constructor to allow moving it.
Note that PointerUnion::{is,get} have been soft deprecated in
PointerUnion.h:
// FIXME: Replace the uses of is(), get() and dyn_cast() with
// isa<T>, cast<T> and the llvm::dyn_cast<T>
I'm not touching PointerUnion::dyn_cast for now because it's a bit
complicated; we could blindly migrate it to dyn_cast_if_present, but
we should probably use dyn_cast when the operand is known to be
non-null.
This pull request corrects multiple occurrences of the typo "avaliable"
to "available" across the LLVM and Clang codebase. These changes improve
the clarity and accuracy of comments and documentation. Specific
modifications are in the following files:
1. clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp:
Updated comments in readability checks for cognitive complexity.
2. llvm/include/llvm/ExecutionEngine/Orc/ExecutionUtils.h: Corrected
documentation for JITDylib responsibilities.
3. llvm/include/llvm/Target/TargetMacroFusion.td: Fixed descriptions for
FusionPredicate variables.
4. llvm/lib/CodeGen/SafeStack.cpp: Improved comments on DominatorTree
availability.
5. llvm/lib/Target/RISCV/RISCVSchedSiFive7.td: Enhanced resource usage
descriptions for vector units.
6. llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp: Updated invariant
description in shift-detect idiom logic.
7. llvm/test/MC/ARM/mve-fp-registers.s: Amended ARM MVE register
availability notes.
8. mlir/lib/Bytecode/Reader/BytecodeReader.cpp: Adjusted forward
reference descriptions for bytecode reader operations.
These changes have no impact on code functionality, focusing solely on
documentation clarity.
Co-authored-by: wangqiang <wangqiang1@kylinos.cn>
Recently there was a change to materializing unrealized conversion
casts, which inserted conversion that previously did not exist during
legalization (https://github.com/llvm/llvm-project/pull/97903), after
these cases are inserted and then washed away after transformation
completes, it caused the use-list ordering of an op to change in some
cases: `my.add %arg0(use1), %arg0(use2) --> my.add %arg0(use2),
%arg0(use1)`, which subtly changes the bytecode emitted since this is
considered a custom use-list.
When investigating why the bytecode had changed I added the following
logging which helped track down the difference, in my case it showed
extra bytes with "use-list section". With
`-debug-only=mlir-bytecode-writer` emits logs like the following,
detailing the source of written bytes:
```
emitBytes(4b) bytecode header
emitVarInt(6) bytecode version
emitByte(13) bytecode version
emitBytes(17b) bytecode producer
emitByte(0) null terminator
emitVarInt(2) dialects count
...
emitByte(5) dialect version
emitVarInt(4) op names count
emitByte(9) op names count
emitVarInt(0) dialect number
...
emitVarInt(2) dialect writer
emitByte(5) dialect writer
emitVarInt(9259963783827161088) dialect APInt
...
emitVarInt(3) attr/type offset
emitByte(7) attr/type offset
emitByte(3) section code
emitVarInt(18) section size
...
```
Note: this uses string constants and `StringLiteral`, I'm not sure if
these are washed away during compilation / OK to have these around for
debuggin, or if there's a better way to do this? Alternative was adding
many braces and `LLVM_DEBUG` calls at each callsite, but this felt more
error prone / likely to miss some callsites.
`StringSectionReader`, `ResourceSectionReader` and
`PropertiesSectionReader` are immutable after `initialize` so this PR
adds const to their parsing functions and references in `AttrTypeReader`
and `DialectReader`.
If the bytecode encoding supports properties, then the dictionary
attribute is always the raw dictionary attribute of the operation,
regardless of what it contains. Otherwise, get the dictionary attribute
from the op: if the op does not have properties, then it returns the raw
dictionary, otherwise it returns the combined inherent and discardable
attributes.
When roundtripping to bytecode an unregistered operation name that does
not contain any '.' separator, the bytecode writer will emit an op
encoding without a proper opName. In this case, the string just becomes
a possibly unknown dialect name. At parsing, this dialect name is
used as a proper operation name.
However, when the unregistered operation name coincidentally matches
that of a dialect, the parser would fail. That means we can't roundtrip
an unregistered op with a name that matches one of the registered
dialect names. For example,
```
"index"() : () -> ()
```
can be emitted but cannot be parsed, because its name is coincidentally
the same as that of the Index dialect. The patch removes such
inconsistency.
This patch specifically fixes the bytecode roundtrip of
`mlir/test/IR/parser.mlir`.
The change in c1eab57 fixed the
behavior of `getDiscardableAttrDictionary` for ops that are not using
properties to only return discardable attributes. Bytecode writer was
relying on the wrong behavior and would assume all attributes are
discardable, without appropriate testing. Fix that and add a test.
This patch replaces uses of StringRef::{starts,ends}with with
StringRef::{starts,ends}_with for consistency with
std::{string,string_view}::{starts,ends}_with in C++20.
I'm planning to deprecate and eventually remove
StringRef::{starts,ends}with.
When serializing to bytecode, users can select the option to elide
resources from the bytecode file. This will instruct the bytecode writer
to serialize only the key and resource kind, while skipping
serialization of the data buffer. At parsing, the IR is built in memory
with valid (but empty) resource handlers.
When emitting bytecode, clients can specify a target dialect version to
emit in `BytecodeWriterConfig`. This exposes a target dialect version to
the DialectBytecodeWriter, which can be queried by name and used to
back-deploy attributes, types, and properties.
This partially reverts #66380. The assertion that the underlying buffer
of an EncodingReader is aligned to any required alignments for resource
sections. Resources know their own alignment and pad their buffers
accordingly, but the bytecode reader doesn't know that ahead of time.
Consequently, it cannot give the resource EncodingReader a base buffer
aligned to the maximum required alignment.
A simple example from the test fails without this:
```mlir
module @TestDialectResources attributes {
bytecode.test = dense_resource<resource> : tensor<4xi32>
} {}
{-#
dialect_resources: {
builtin: {
resource: "0x2000000001000000020000000300000004000000",
resource_2: "0x2000000001000000020000000300000004000000"
}
}
```
Before this change, the `ByteCode` test failed on CentOS 7 with
devtoolset-9, because strings happen to be only 8 byte aligned. In
general though, strings have no alignment guarantee.
Increase resource alignment in test to 32 bytes.
Adjust test to sufficiently align buffer.
Add test to check error when buffer is insufficiently aligned.
Leaving this field unitialized could led to crashes when it'll diverge from the
IRNumbering phase.
Differential Revision: https://reviews.llvm.org/D156965
[mlir] Expose a mechanism to provide a callback for encoding types and attributes in MLIR bytecode.
Two callbacks are exposed, respectively, to the BytecodeWriterConfig and to the ParserConfig. At bytecode parsing/printing, clients have the ability to specify a callback to be used to optionally read/write the encoding. On failure, fallback path will execute the default parsers and printers for the dialect.
Testing shows how to leverage this functionality to support back-deployment and backward-compatibility usecases when roundtripping to bytecode a client dialect with type/attributes dependencies on upstream.
Reviewed By: rriddle
Differential Revision: https://reviews.llvm.org/D153383
[mlir] Expose a mechanism to provide a callback for encoding types and attributes in MLIR bytecode.
Two callbacks are exposed, respectively, to the BytecodeWriterConfig and to the ParserConfig. At bytecode parsing/printing, clients have the ability to specify a callback to be used to optionally read/write the encoding. On failure, fallback path will execute the default parsers and printers for the dialect.
Testing shows how to leverage this functionality to support back-deployment and backward-compatibility usecases when roundtripping to bytecode a client dialect with type/attributes dependencies on upstream.
Reviewed By: rriddle
Differential Revision: https://reviews.llvm.org/D153383
Zero region operations return true for both isBeforeAllRegions and
isAfterAllRegions when using WalkStage. The bytecode walk only
expects region holding operations in the after regions path, so
guard against that.
We currently only support lazy loading for regions that
statically implement the IsolatedFromAbove trait, but that
limits the amount of operations that can be lazily loaded. This review
lifts that restriction by computing which operations have isolated
regions when numbering, allowing any operation to be lazily loaded
as long as it doesn't use values defined above.
Differential Revision: https://reviews.llvm.org/D156199
We currently encode each region as a separate section, but
the reader expects all of the regions to be in the same section.
This updates the writer to match the behavior that the reader
expects.
Differential Revision: https://reviews.llvm.org/D156198
