Conversation
There was a problem hiding this comment.
Pull request overview
Adds an “execution policy” feature to the simulator so core instruction issue can be gated by different scheduling semantics (strict/elastic/in-order), configurable via arch spec, with tests validating normalization and behavior.
Changes:
- Add
simulator.execution_policyto arch spec parsing/resolution (defaulting toin_order_dataflowwith aliases). - Plumb execution policy into device/core construction and enforce it in the core emulator at issue time.
- Add unit/integration tests and a new policy-behavior scenario YAML.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/testbench/policy_behavior/policy_behavior_test.go | Integration test exercising strict vs elastic vs in-order behavior on late-arrival scenario. |
| test/testbench/policy_behavior/late_arrival.yaml | New minimal 2-tile scenario to trigger/avoid synchronization violations depending on policy. |
| test/arch_spec/arch_spec.yaml | Adds execution_policy to the example/test arch spec. |
| runtimecfg/spec.go | Extends Simulator spec struct to accept execution_policy. |
| runtimecfg/runtime_test.go | Tests default policy, alias normalization, and invalid policy handling. |
| runtimecfg/runtime.go | Adds resolved ExecutionPolicy, defaulting + normalization, and passes it into device build. |
| core/program.go | Persists YAML time_step into core.Operation for scheduling decisions. |
| core/execution_policy_test.go | Unit tests for canIssue under different policies + YAML parsing timestep preservation. |
| core/emu.go | Implements execution-policy-aware canIssue and synchronization-violation diagnostics. |
| core/core.go | Tracks CurrentCycle per core to support time-step based issue gating. |
| core/builder.go | Accepts execution policy in core builder and sets it on the emulator. |
| config/config.go | Adds device builder plumbing for execution policy into core construction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // WithExecutionPolicy sets core execution policy. | ||
| func (d DeviceBuilder) WithExecutionPolicy(policy string) DeviceBuilder { | ||
| d.executionPolicy = policy | ||
| return d | ||
| } |
There was a problem hiding this comment.
WithExecutionPolicy accepts any string and stores it without validation/normalization. Combined with core-side silent fallback on unknown policies, this can make misconfigurations hard to detect. Consider validating/normalizing here (or changing the API to a typed enum/consts) so invalid values are rejected early and behavior matches runtimecfg.Resolve validation.
|
|
||
| return currentStep, targetStep, ii | ||
| } | ||
|
|
||
| func (i instEmulator) canIssue(operation Operation, state *coreState) bool { | ||
| if !i.CareFlags || operation.InvalidIterations > 0 { | ||
| return true | ||
| } | ||
|
|
There was a problem hiding this comment.
canIssue always calls resolveScheduleStep, which can panic when CompiledII > 0 and operation.TimeStep is out of range. That means in_order_dataflow (which is supposed to ignore time-step scheduling) can still panic purely due to schedule validation, and it also does extra work on every issue check. Consider only resolving/validating schedule steps inside the strict_timed / elastic_scheduled branches (or gating it by policy) so in_order_dataflow truly ignores TimeStep and cannot fail because of it.
| filePath := "../test/testbench/stonneGEMM8x8/gemm.yaml" | ||
| if _, err := os.Stat(filePath); os.IsNotExist(err) { | ||
| t.Skipf("test file does not exist: %s", filePath) | ||
| } | ||
|
|
||
| programMap := LoadProgramFileFromYAML(filePath) |
There was a problem hiding this comment.
TestLoadProgramFileFromYAMLPreservesTimeStep references ../test/testbench/stonneGEMM8x8/gemm.yaml, but that path does not exist in the repo, so this test will always Skip and won’t provide coverage. Point it at an existing instruction YAML in test/testbench (e.g. one of the tmp-generated-instructions.yaml files that includes time_step) or inline a minimal YAML fixture in the test.
| filePath := "../test/testbench/stonneGEMM8x8/gemm.yaml" | |
| if _, err := os.Stat(filePath); os.IsNotExist(err) { | |
| t.Skipf("test file does not exist: %s", filePath) | |
| } | |
| programMap := LoadProgramFileFromYAML(filePath) | |
| // Inline minimal YAML fixture to ensure this test does not depend on external files. | |
| const yamlContent = ` | |
| "(0,0)": | |
| entry_blocks: | |
| - instruction_groups: | |
| - operations: | |
| - op_code: "NOP" | |
| time_step: 0 | |
| - operations: | |
| - op_code: "NOP" | |
| time_step: 1 | |
| ` | |
| tmpFile, err := os.CreateTemp("", "program-*.yaml") | |
| if err != nil { | |
| t.Fatalf("failed to create temp file: %v", err) | |
| } | |
| defer os.Remove(tmpFile.Name()) | |
| if _, err := tmpFile.Write([]byte(yamlContent)); err != nil { | |
| tmpFile.Close() | |
| t.Fatalf("failed to write YAML content to temp file: %v", err) | |
| } | |
| if err := tmpFile.Close(); err != nil { | |
| t.Fatalf("failed to close temp file: %v", err) | |
| } | |
| programMap := LoadProgramFileFromYAML(tmpFile.Name()) |
Jackcuii
left a comment
Jackcuii
left a comment
There was a problem hiding this comment.
LGTM, but please run fir and relu to ensure they are right before we merge it. @n0thingNoob
No description provided.