Skip to content

FlatExpression: split parameter decl/usage, add parent-linked chains, and introduce generic node references#539

Closed
Copilot wants to merge 7 commits intomasterfrom
copilot/optimize-flat-expression-representation-another-one
Closed

FlatExpression: split parameter decl/usage, add parent-linked chains, and introduce generic node references#539
Copilot wants to merge 7 commits intomasterfrom
copilot/optimize-flat-expression-representation-another-one

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 27, 2026

FlatExpression needed a representation update for upcoming FE work: parameter identity/scope metadata should be encoded directly in nodes, and child-chain links should support upward navigation without complicating traversal. This PR reshapes node metadata and linking semantics accordingly while preserving constant inlining/closure behavior.

  • Parameter model: declaration vs usage

    • Added ExprNodeKind.ParameterDeclaration and ExprNodeKind.ParameterUsage.
    • ExprTree.Parameter(...) now creates declaration nodes.
    • Parameter usages are emitted as separate nodes with ChildIdx -> declarationIndex, while keeping Type/Name/flags for cheap checks.
    • Lambda/block construction now stamps declaration ownership:
      • ChildIdx = owner scope index (lambda/block)
      • ChildCount = parameter/variable position in owner
    • Re-binding an already-owned declaration fails fast.
  • Child-chain/parent linkage model

    • Added sibling-vs-parent distinction in link metadata.
    • The last node in each child chain now stores parent link (NextIdx -> parent + parent-link bit).
    • Internal traversal semantics remain sibling-first; parent links are now available for upward navigation.
  • General reference-node optimization

    • Added ExprNodeKind.NodeReference for reusable child nodes.
    • First use of a node keeps the original node in place.
    • If a node is already linked, subsequent uses emit a lightweight reference node (ChildIdx -> original).
    • Includes reservation metadata to handle same-parent repeated arguments (Add(x, x)) without eager cloning.
  • Builder/reader integration

    • System-expression flattening now builds declaration/usage nodes consistently (including lambda/block declaration stamping).
    • Reader resolves NodeReference indirection and maps parameter identity by declaration index.
  • Focused coverage updates

    • Added tests validating:
      • declaration/usage split + owner/position stamping
      • parent link on last child
      • reference-node emission on reuse
var p = fe.Parameter(typeof(int), "p");      // declaration
var body = fe.Add(p, p);                     // usages emitted on linking
fe.RootIndex = fe.Lambda<Func<int, int>>(body, p);

// declaration metadata written by Lambda(...)
var decl = fe.Nodes[p]; // Kind=ParameterDeclaration, ChildIdx=lambdaIndex, ChildCount=0

Copilot AI linked an issue Apr 27, 2026 that may be closed by this pull request
Copilot AI changed the title [WIP] Optimize FlatExpression representation for performance improvements FlatExpression: split parameter decl/usage, add parent-linked chains, and introduce generic node references Apr 27, 2026
Copilot AI requested a review from dadhi April 27, 2026 15:43
Copy link
Copy Markdown
Owner

@dadhi dadhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CsCheck failed:

untu-latest)
failed 23 minutes ago in 1m 11s
1s
1s
10s
55s
2s
Run dotnet run --no-build -c:Release -f:net10.0 --project test/FastExpressionCompiler.TestsRunner/FastExpressionCompiler.TestsRunner.csproj

//## .NET Latest (Core): Running UnitTests and IssueTests in parallel...

ERROR: Tests FastExpressionCompiler.LightExpression.UnitTests.LightExpressionTests failed with
CsCheck.CsCheckException: Set seed: "eaAXCWRSvcE1" or -e CsCheck_Seed=eaAXCWRSvcE1 to reproduce (5 shrinks, 52 skipped, 100 total).
164763702: Multiply { Left = Multiply { Left = LetMany { Values = FastExpressionCompiler.LightExpression.UnitTests.LightExpressionTests+IntSpec[], Body = ParameterRef { Index = 2 } }, Right = ParameterRef { Index = 0 } }, Right = Constant { Value = -1 } }
Node at index 14 is not an expression node.
at FastExpressionCompiler.FlatExpression.ExprTree.Reader.ReadExpression(Int32 index) in //src/FastExpressionCompiler.LightExpression/FlatExpression.cs:line 1362
at FastExpressionCompiler.FlatExpression.ExprTree.Reader.ReadExpression(Int32 index) in /
/src/FastExpressionCompiler.LightExpression/FlatExpression.cs:line 1399

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 27, 2026

CsCheck failed:

untu-latest)...

Fixed in 2517e7c. The regression was in ExprTree.Block(...): block body children were linked without CloneChild, so parameter declaration nodes could be linked into multiple child chains and corrupt traversal. Body children are now cloned on insert, and the reported CsCheck seed (eaAXCWRSvcE1) passes locally via FastExpressionCompiler.TestsRunner net10.0.

Copilot AI requested a review from dadhi April 27, 2026 17:45
Copy link
Copy Markdown
Owner

@dadhi dadhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we replace Param usage node eith node reference and simplify?

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 27, 2026

Can we replace Param usage node eith node reference and simplify?

Not safely in this PR as-is. ParameterUsage is intentional for explicit decl-vs-usage metadata in-node (cheap checks, scope/identity semantics) while declaration nodes keep owner/position stamping. A plain NodeReference would make usage implicit via indirection and weaken that model. I can do a follow-up refactor if we want to change the model to “declaration + reference-only usages” end-to-end.

Copilot AI requested a review from dadhi April 27, 2026 18:07
Copy link
Copy Markdown
Owner

@dadhi dadhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Will it simplify the bit manipulation and the code if we split data into short NodeTypeAndKind; short NextId, uint _data?
  • For the remaining bit manipulations please add illustrative technical comment about what it does, usually bit its extracts or sets.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 28, 2026

  • Will it simplify the bit manipulation and the code if we split data into short NodeTypeAndKind; short NextId, uint _data?
  • For the remaining bit manipulations please add illustrative technical co...

Good point. I kept the current packed ulong _data layout for this PR to avoid changing node size/layout semantics broadly, but I added illustrative technical comments for the remaining bit operations (layout, extraction, and set paths) in ExprNode in d90ce7e.

Copilot AI requested a review from dadhi April 28, 2026 09:35
Copy link
Copy Markdown
Owner

@dadhi dadhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split // [ 8 bits NodeType ][ 8 bits Tag(flags+kind) ][ 16 bits NextIdx ]
in the separate fields if those are the stable and not reused to store something crossing the boundaries of byte + byte + word.
Simplify the bit manipulation and update the usage and test accordignly.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 28, 2026

Please split // [ 8 bits NodeType ][ 8 bits Tag(flags+kind) ][ 16 bits NextIdx ]
in the separate fields if those are the stable and not reused to store something crossing the boundaries of byte + by...

Done in 02fdc70. I split the stable packed header into explicit fields (_nodeType, _tag, _nextIdx) and simplified usage accordingly (plus explicit _childIdx/_childCount), keeping the struct size/layout contract (Size = 24) and semantics intact. Tests pass via FastExpressionCompiler.TestsRunner net10.0. No UI changes in this PR, so no screenshot applies.

Copilot AI requested a review from dadhi April 28, 2026 10:45
Copy link
Copy Markdown
Owner

@dadhi dadhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I am not sure now how do we store the struct value constant up to 4 bytes in place, and why are we still using ClosureConstantMarker object and not some kinf of flag?

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 28, 2026

Ok. I am not sure now how do we store the struct value constant up to 4 bytes in place, and why are we still using ClosureConstantMarker object and not some kinf of flag?

Updated in 105f73e. Struct/primitives that pass ShouldInlineConstant are kept inline in ExprNode.Obj (boxed for value types; added a code comment at that check). For non-inlined constants, I replaced ClosureConstantMarker with an explicit node flag (ConstantInClosureFlag) and now resolve via node.HasFlag(...) + ChildIdx into ClosureConstants, so we no longer need the sentinel object. No UI changes in this PR, so no screenshot applies.

@dadhi
Copy link
Copy Markdown
Owner

dadhi commented Apr 29, 2026

Bs

@dadhi dadhi closed this Apr 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants