[OM] Fix object field value is considered fully evaluated even it's not#10293
[OM] Fix object field value is considered fully evaluated even it's not#10293
Conversation
seldridge
left a comment
seldridge
left a comment
There was a problem hiding this comment.
I have a couple of questions. This looks great, though. The two worklist approach makes a lot of sense and simplifies the cycle detection.
| // Test for issue #10264 - nested object field references that previously | ||
| // caused an assertion failure. This test verifies that the evaluator can | ||
| // properly handle nested field accesses without hitting null pointers. |
There was a problem hiding this comment.
Nit: Please include a direct link tot he GitHub issue. Also, please rewrite this LLM-generated comment.
There was a problem hiding this comment.
Is there a policy for preferring a direct link over issue number? I don't have a strong opinion but certainly I've used both #<issue_number> and URL without much consideration. Certainly LLVM also doesn't have a policy for that, can you add to CIRCT coding guideine?
circt/test/Dialect/FIRRTL/legacy-wiring.mlir
Lines 212 to 213 in babde3d
https://github.com/llvm/llvm-project/blob/5892e34a96131821256f89c0555ba68c23c91dbe/mlir/test/Analysis/test-match-reduction.mlir#L118
There was a problem hiding this comment.
Sure. I can do that.
There's a significant trend towards full links, but both are used:
# grep -r "\(;\|//\).*#[0-9]\+" ../test | grep -v "\(CHECK\|expected\|CONN\)" | wc -l
55
# grep -r "https://github.com/llvm/circt/issues" ../test | wc -l
237
| if (succeeded(result)) | ||
| attachCounter(result.value()); |
There was a problem hiding this comment.
Given the checking in attachCounter, can this blindly call attachCounter(result.value())? I guess not given that the return type is FailureOr<...>? result.value() must then auto-unpack the FailureOr for you?
There was a problem hiding this comment.
I think result.value() causes assertion failure for FailureOr::failuare(= std::nullopt), so we need to add a guard for this.
| auto boolValue = llvm::cast<evaluator::AttributeValue>(fieldValue.get()) | ||
| ->getAs<BoolAttr>(); | ||
| ASSERT_FALSE(boolValue.getValue()); | ||
| } |
There was a problem hiding this comment.
Very happy to see this working. Nice!
2 participants
This fixes an issue that object field is considered as evaluated even when it's still pending. The fix itself was only:
However this change forces a more proper cycle detection mechanism for dataflow cycle, so this PR also added a counter that tracks the number of fully evaluated values. I'm hoping to replace these fixes with more simpler solution that actually mutates the IR as mentioned in #10265
Fixes #10264.
Assisted-by: Augment: claude-sonnet-4.5