[OM] Introduce explicit evaluator state and op patterns by uenoku · Pull Request #10265 · llvm/circt

uenoku · 2026-04-18T05:05:23Z

Refactor the OM evaluator around explicit Ready/Pending/Failure state tracking and a declarative operation pattern framework which follows similar structure as mlir's RewritePattern.

Previously each evaluation needed to prevent every corner cases (attributes/reference being unknown, reference, pending) etc. This tries to prevents that by using OpReadyOperandsPattern.

This data structure/framework should make it difficult to introduce a bug, and as a side effect it fixes #10264.

My intention is mostly NFCI except for fixing missing error/partially evaluated value handling, but will need tests for real designs.
Also add the tests for unknown nested fields and reference/value propagation.

Assisted-by: Codex: GPT-5.4

Refactor the OM evaluator around explicit Ready/Pending/Failure state tracking and a shared operation pattern framework. Centralize reference resolution, operand readiness/unknown propagation, and placeholder creation, while moving op-specific logic into typed patterns. Also add the supporting evaluator regressions for unknown nested fields and reference/value propagation.

uenoku · 2026-04-18T17:34:03Z

+  auto result = createUnknownValue(op.getType(), loc);
+  if (failed(result))
+    return ResolvedValue::failure();
+  return resolveValueState(result.value());


Suggested change

auto result = createUnknownValue(op.getType(), loc);

if (failed(result))

return ResolvedValue::failure();

return resolveValueState(result.value());

return createUnknownValue(op.getType(), loc);

uenoku · 2026-04-18T17:34:25Z

-  return evaluateObjectInstance(op.getClassNameAttr(), params.value(), loc,
-                                {op, actualParams});
+    return ResolvedValue::failure();
+  auto result = evaluateObjectInstance(op.getClassNameAttr(), params.value(),


Suggested change

auto result = evaluateObjectInstance(op.getClassNameAttr(), params.value(),

return evaluateObjectInstance(op.getClassNameAttr(), params.value(),

uenoku · 2026-04-18T21:00:13Z

+    foldedAttr = attr;
+  } else
+    return op->emitError(
+        "folder returned operands even though all operands are constant, "


Comment for link.

uenoku · 2026-04-19T00:02:18Z

Another direction might be to switch to clone IR and actually mutate/flatten IR instead of using in-memory graph. In this approach we can completely eliminate the notion of scheduling (after flattening all objects there should not be any reference). In this case we can implement the evaluation system with (greedy) pattern rewriter nicely.

seldridge · 2026-04-20T14:39:32Z

+/// The evaluator tracks two different things:
+///
+///   1. Local state of one value object: `isSettled()`
+///
+///      false -> this value object may still change
+///      true  -> this value object has finished its own local work
+///
+///   2. State of using a value handle: `ResolutionState`
+///
+///      Pending -> the handle still cannot be used
+///      Ready   -> the handle can be used now
+///      Failure -> evaluation hit a hard error
+///
+/// These are not the same thing. A reference may itself be settled, but using
+/// the handle may still be pending:
+///
+///   ReferenceValue (settled = true)
+///          |
+///          v
+///      pending target
+///
+/// So `isSettled()` is about one value object, while `ResolvedValue` is about
+/// whether the whole handle is usable.


I'm not following this... So, if a value is settled and it has a single user, under what circumstances would the mean that the user is not ready? Is this only an accounting issue of the user needs to be updated from pending (it's current state) to ready (it's next state)?

Yes, that's possible under the current complicated state transition, unfortunately mostly because of Referencevalue.

Fox example if reference value point to add(x, y) reference value itself would be settled when the reference value reaches add(x, y). However add(x, y) may not be fully evaluated, so referecen(add(x, y)) is settled but not ready.

I think the majority of complexity came from (1) the current in-memory representation about reference value, (2) object field allowing nested fields.

So I'm inclined to pursue the approach to actually mutate the IR, instead of using in-memory representation (#10265 (comment)) since we cannot avoid this complicated scheduling otherwise. This file is the IR I'm experimenting (https://github.com/uenoku/circt/blob/dev/hidetou/evaluator-framework-2/test/Dialect/OM/elaborate-object.mlir) that basically introduces om.elaborated_object operation (https://github.com/uenoku/circt/blob/ebc4122876cb184e193cd7e02631d51b08a6dbcb/include/circt/Dialect/OM/OMOps.td#L209).

I kind of like the IR mutation strategy only because it's simpler and I can therefore understand it (and it doesn't require lots of debugging logging to figure out when it goes wrong). 😅

Yes, I think that's better. Though it's necessary to check that the IR mutation approach properly scales for large design, as essentially it flattens entire objects into a single block.

Yes, fair. It has to visit every object uniquely. It is possible that the final structure could be very compact, though, as long as unused/unknown structure is discarded.

seldridge · 2026-04-20T14:50:20Z

+struct ResolvedValue {
+  /// `state` says whether `value` can be used. `value` keeps the original
+  /// handle so callers can keep passing it around even when it is still pending
+  /// or has already failed.
+  ResolutionState state;
+  EvaluatorValuePtr value;


This is looking a lot like llvm::PointerUnion or llvm::PointerIntPair.

I realize that EvaluatorValuePtr is using std::shared_ptr which makes this likely not workable. There may be an alternative formulation that can be more memory efficient here.

seldridge · 2026-04-20T15:16:28Z

+  auto result = op.evaluateIntegerOperation(lhsVal, rhsVal);
+  if (failed(result))
+    return {};


Maybe the indirection through foldIntegerBinaryArithmetic and evaluateIntegerOperation could just be removed here in favor of directly evaluating the fold member function, similar to how FIRRTL IMCP is implemented.

This is going beyond the more pure refactor nature of this PR, though.

seldridge · 2026-04-20T15:22:26Z

+TEST(EvaluatorTests, StringConcatSingleOperand) {
+  const char *mod = R"MLIR(
+module {
+  om.class @Test() -> (result: !om.string) {
+    %0 = om.constant "Hello" : !om.string
+    %1 = om.string.concat %0 : !om.string
+    om.class.fields %1 : !om.string
+  }
+}
+)MLIR";


Nit/thought: this isn't consistent with the other tests in this file, but it may be cleaner to batch all of the string concat tests into one Google Test TEST (StringConcat) and then do multiple ASSERT_* calls in that.

seldridge · 2026-04-20T15:25:31Z

+  auto fieldValue = llvm::cast<evaluator::ObjectValue>(result.value().get())
+                        ->getField("result")
+                        .value();
+
+  ASSERT_EQ("Hello", llvm::cast<evaluator::AttributeValue>(fieldValue.get())
+                         ->getAs<StringAttr>()
+                         .getValue());


Nit: the pattern of llvm::cast<evaluator::ObjectValue>(...)->getField(...).value() followed by a whole additional cast to an attribute value, feels like there is a missing templated getFieldAs<T> to allow for getFieldAs<StringAttr> to avoid some of the long cast chains.

Unrelated to the PR, though.

seldridge · 2026-04-20T15:33:41Z

+om.class @Domain(%in: !om.string) -> (out: !om.string) {
+  om.class.fields %in : !om.string
+}
+om.class @Foo_Class(%basepath: !om.frozenbasepath) -> (test1: i1, test2: i1) {


Nit: This can just be @Foo. The @Foo_Class is only really relevant if testing the "FIRRTL Property ABI". I do realize that this is my copy-pasted issue. 😉

…ot (#10293) This fixes an issue that object field is considered as evaluated even when it's still pending. The fix itself was only: ``` if (!finalField->isFullyEvaluated()) return objectFieldValue; ``` 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

…or, extend StringConcat folder (#10269) This extends IntegerBinaryArithmeticOp folder using evaluation rule in Evaluator. Also changes Evaluator to use folder to avoid code duplication. Also extends a folder of StringConcat. Separated from #10265, which proposes to a pattern to use Op folder not only IntegerArithmeticOp. Assisted-by: Codex: GPT 5.4

uenoku force-pushed the dev/hidetou/evaluator-framework branch 2 times, most recently from 3483b6c to 75fd19d Compare April 18, 2026 05:45

uenoku force-pushed the dev/hidetou/evaluator-framework branch from 75fd19d to 6123679 Compare April 18, 2026 05:51

seldridge mentioned this pull request Apr 18, 2026

[OM] Delay assertions until worklist drained #10267

Merged

Rename terminology, add a doc

92256fe

uenoku force-pushed the dev/hidetou/evaluator-framework branch from af62281 to 55bdc02 Compare April 18, 2026 07:54

Use a folder directly

a82bb7b

uenoku force-pushed the dev/hidetou/evaluator-framework branch from 55bdc02 to a82bb7b Compare April 18, 2026 08:24

uenoku commented Apr 18, 2026

View reviewed changes

uenoku mentioned this pull request Apr 18, 2026

[OM] Add a folder for IntegerBinaryArithmeticOp and use it in Evaluator, extend StringConcat folder #10269

Merged

uenoku commented Apr 18, 2026

View reviewed changes

uenoku force-pushed the dev/hidetou/evaluator-framework branch from 651b934 to a82bb7b Compare April 20, 2026 09:07

seldridge reviewed Apr 20, 2026

View reviewed changes

uenoku mentioned this pull request Apr 21, 2026

[OM] Fix object field value is considered fully evaluated even it's not #10293

Merged

uenoku mentioned this pull request Apr 23, 2026

[OM] Clean up ObjectOfieldOp: Move away from symbols, remove a field path and VerifyObjectFields pass #10303

Merged

	auto result = evaluateObjectInstance(op.getClassNameAttr(), params.value(),
	return evaluateObjectInstance(op.getClassNameAttr(), params.value(),

Conversation

uenoku commented Apr 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uenoku commented Apr 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

uenoku commented Apr 18, 2026 •

edited

Loading