Fix non-deterministic constant folding output - #29789
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
After fix: |
|
@tianleiwu |
tianleiwu
left a comment
There was a problem hiding this comment.
Thanks for the fix — the root-cause analysis is correct and the change is minimal and well-targeted.
Why this is right: OwnsBuffer() returns buffer_deleter_ != nullptr. Alias-producing ops (Reshape/Unsqueeze/Squeeze/Flatten/Identity) return non-owning tensors that view an input initializer's buffer, so OwnsBuffer()==false and the data is now deep-copied inline instead of being referenced as in-memory external data that dangles once the parent initializer is removed. For owned outputs, the zero-copy fast path is preserved and the stored ort_value keeps the buffer alive. Small tensors (≤127B) were already inlined, so the fix targets exactly the affected case. The updated comment on the HasExternalData branch also now matches the new behavior.
Non-blocking suggestion: consider adding a regression test (see inline comment) so a future revert to unconditional use_tensor_buffer=true is caught automatically. Otherwise this looks good to me.
|
This bug actually affects likely every ONNX model because almost every deep learning model uses alias-producing ops like For instance, I just tested it with D-FINE and DEIMv2. When With the fix, the outputs are stable and deterministic (same as |
|
@ducviet00 thanks for address the issue. I'll add a test case later. |
Description
From
v1.24.1,use_tensor_buffer=trueis set inTensorToTensorProtoduring constant folding. For ops that return aliased OrtValues (e.g. Reshape), the folded initializer referenced memory owned by an input initializer. When that input was removed later in the pass, the reference dangled, producing unstable (often huge/negative) values.Use
out_tensor.OwnsBuffer()to decideuse_tensor_buffer. Owned tensors are referenced zero-copy (safe viaOrtValueshared lifetime); aliased tensors are deep-copied into the protoRelated issue
#29788