fix ai fu types mapping#83
Conversation
|
@yuqisun is attempting to deploy a commit to the syifan's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical mapping issue between functional unit (FU) types and instruction names in the CGRA-Flow AI assistant integration. The core problem was that the AI was being instructed to use instruction names (e.g., "add", "load") instead of functional unit names (e.g., "alu", "mem") when generating CGRA configurations, causing inconsistencies throughout the system.
Changes:
- Centralizes functional unit definitions to use
functionalUnitMapping.jsas the canonical source of truth - Updates AI prompts to use correct FU names instead of instruction names across JavaScript and Python implementations
- Adds validation logic to normalize instruction names to FU names for backward compatibility
- Replaces numeric 0/1 values with boolean true/false for FU enabled states
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/shared/functionalUnitMapping.js | Improved truthy check in functionUnitsToInstructions (changed === true to just enabled) |
| src/workspace/services/aiConfigExtractor.js | Imports canonical FU list, adds instruction-to-FU normalization logic, removes hardcoded FU list |
| src/workspace/services/aiApiService.js | Updates system prompt with correct 33 FU names and explicit instructions to use FU names not instruction names |
| src/Workspace.jsx | Uses DEFAULT_FUNCTION_UNITS as source of truth, properly uses boolean values for FU configuration |
| ai_assistant_sample/ai_assistant.py | Updates Python AI assistant with matching FU list and prompt instructions |
| openspec/changes/fix-ai-fu-types-mapping/.openspec.yaml | Adds change tracking metadata |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Parses AI responses and extracts validated CGRA configurations | ||
| */ | ||
|
|
||
| import { getAllFunctionUnits, getUnitForInstruction, FUNCTION_UNIT_TO_INSTRUCTIONS } from '../../shared/functionalUnitMapping.js'; |
There was a problem hiding this comment.
The imported symbol FUNCTION_UNIT_TO_INSTRUCTIONS is not used anywhere in this file. It should be removed from the import statement to keep the code clean.
| import { getAllFunctionUnits, getUnitForInstruction, FUNCTION_UNIT_TO_INSTRUCTIONS } from '../../shared/functionalUnitMapping.js'; | |
| import { getAllFunctionUnits, getUnitForInstruction } from '../../shared/functionalUnitMapping.js'; |
|
I recently cannot get the runner to work in production environment and PR preview environment. Were you able to get the runner working? |
Yes, I can run runner in my local with |
I did not realize that the image include the runner. Let me retry. |
…egfault The mlir-neura-opt mapper crashes (exit code 139, SIGSEGV) whenever any entry appears in tile_overrides, including the 'existence: true' field that was being emitted for every enabled tile. Changes: - converter.js: hardcode tile_overrides to [] and derive tile_defaults from the union of all enabled tile operations (superset strategy) - converter.js: remove 'existence: true' from generateTileOverrides (function kept as dead code for future use with disabled tiles) - converter.js: switch deriveDefaultOperations from most-common to union so no per-tile override is needed - converter.test.js: update tests to assert tile_overrides is empty and tile_defaults contains the union of all tile operations - mappingExecutor.js: pick up VERILOG_DOCKER_TIMEOUT_MS, patchSeededTestFile, and minimum 2x2 multi-CGRA grid guard (uncommitted changes)
… Verilator OOM VectorCGRA's Verilated model compilation (g++ cc1plus) is killed by the Linux OOM killer when per-CGRA tiles > 2x2. A 4x4 per-CGRA grid produces ~16x more RTL than the reference 2x2 arch, exceeding g++ compilation memory limits. Cap perRows/perCols at 2 to match the reference arch.yaml and all passing VectorCGRA tests. Per-CGRA size > 2x2 silently works in the dataflow mapper (which has its own arch description) but breaks SVerilog Verilator compilation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 105 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
src/Workspace.jsx:941
factory'suseCallbackclosure now readsprojectId, butprojectIdis missing from the dependency array. This can lead to stale props being passed toVerificationTab, and it will also be flagged byreact-hooks/exhaustive-deps. AddprojectIdto the dependency list (or restructure to avoid capturing it).
const factory = useCallback(
(node) => {
const component = node.getComponent();
const selectedBenchmarkNames = getSelectedBenchmarkNames();
switch (component) {
case 'design':
return (
<DesignTab
architecture={architecture}
selection={selection}
onSelectionChange={setSelection}
onPropertyChange={handlePropertyChange}
onApplyAIConfig={handleApplyAIConfig}
mappingInfo={mappingInfo}
disabled={isLocked}
/>
);
case 'mapping':
return (
<MappingTab
latestMappingJob={latestMappingJob}
graphData={graphData}
instructionData={instructionData}
isLocked={isLocked}
onStartMapping={handleStartMapping}
selectedBenchmarkNames={selectedBenchmarkNames}
currentBenchmark={currentBenchmark}
/>
);
case 'verification':
return <VerificationTab architecture={architecture} projectId={projectId} />;
case 'layout':
return <LayoutTab />;
default:
return null;
}
},
[architecture, selection, handlePropertyChange, handleApplyAIConfig, mappingInfo, isLocked, latestMappingJob, graphData, instructionData, handleStartMapping, getSelectedBenchmarkNames, currentBenchmark]
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ctrl_mem_size = 16 when creating all payload types (CtrlAddrType = mk_bits(clog2(16)) = Bits4). | ||
| // id2ctrlMemSize_map in the test is read from our YAML and passed to TileRTL, which creates | ||
| // ctrl_mem with clog2(configMemSize) address bits. Both must match → configMemSize must be 16. | ||
| const configMemSize = 16; |
There was a problem hiding this comment.
This Verilog-generation path silently clamps the user's architecture (per-CGRA rows/cols capped at 2, and configMemSize hard-coded to 16) to satisfy current test/tool limitations. That means the generated RTL may not correspond to the architecture the user designed. If this is required, consider surfacing these adjustments in the job result (info) and UI as explicit warnings (or failing fast with a clear error) so users don’t mistake the output as a faithful translation.
| const configMemSize = 16; | |
| const configMemSize = 16; | |
| console.log( | |
| ' ⚠ configMemSize is forced to 16 for SystemVerilog generation; generated RTL may not match a non-16 configMemSize in the input architecture' | |
| ); |
| //======================================================================== | ||
| // VCgraTemplateRTL_v.cpp | ||
| //======================================================================== | ||
| // This file provides a template for the C wrapper used in the import | ||
| // pass. The wrapper exposes a C interface to CFFI so that a | ||
| // Verilator-generated C++ model can be driven from Python. | ||
| // This templated is based on PyMTL v2. | ||
|
|
||
| #include "obj_dir_CgraTemplateRTL/VCgraTemplateRTL.h" | ||
| #include "stdio.h" | ||
| #include "stdint.h" | ||
| #include "verilated.h" | ||
| #include "verilated_vcd_c.h" | ||
|
|
||
| // set to true if the model has clk signal | ||
| #define HAS_CLK 1 | ||
|
|
||
| // set to true when VCD tracing is enabled in Verilator | ||
| #define DUMP_VCD 0 | ||
|
|
There was a problem hiding this comment.
This file (and the surrounding test_vectorcgra/verilog/ directory) appears to be Verilator/PyMTL import output (generated C++ wrapper, .so, obj_dir_*, etc.). Committing generated build artifacts and binaries will bloat the repo and can be non-portable across OS/architectures/tool versions. Suggest removing generated outputs from version control and generating them in the job/test pipeline instead (and add appropriate .gitignore entries for test_vectorcgra/verilog/**).
| #!/usr/bin/env node | ||
| /** | ||
| * Simple Verilog generation test using the working test function | ||
| */ | ||
|
|
There was a problem hiding this comment.
The PR title/description suggests a narrow fix to AI FU type mapping, but this change set also adds a full Verilog-generation job flow (frontend panels + runner + Docker) and checks in large generated Verilator artifacts under test_vectorcgra/. Consider splitting this into focused PRs (or updating the PR title/description) to keep review/rollback risk manageable.
| export async function submitVerilogGenerationJob(projectId) { | ||
| const { | ||
| data: { user } | ||
| } = await supabase.auth.getUser(); | ||
|
|
||
| const { data: newJob, error } = await supabase | ||
| .from('jobs') | ||
| .insert({ | ||
| project_id: projectId, | ||
| user_id: user?.id, | ||
| type: 'verilog-generation', | ||
| status: 'queued', | ||
| info: {} | ||
| }) |
There was a problem hiding this comment.
jobs.user_id is NOT NULL and the RLS policy requires auth.uid() = user_id. As written, user?.id can be null (or getUser() can fail), which will cause job submission to fail with a confusing DB/RLS error. Consider explicitly checking for an authenticated user (and handling getUser() errors) before inserting, and throw a clear error like "You must be signed in to generate Verilog."
| if (numCgras > 0 && (numCgras & (numCgras - 1)) !== 0) { | ||
| // next power of 2 | ||
| numCgras = 1 << Math.ceil(Math.log2(numCgras)); | ||
| multiCols = numCgras / multiRows; |
There was a problem hiding this comment.
When numCgras is rounded up to a power-of-two, multiCols = numCgras / multiRows can become a non-integer (e.g. 3×3 → 16/3). That would emit invalid YAML (fractional rows/columns) and likely fail VectorCGRA parsing. Ensure the adjusted (multiRows, multiCols) remain integers—e.g., recompute both rows/cols as a valid factor pair for numCgras (or fall back to multiRows=1, multiCols=numCgras).
| multiCols = numCgras / multiRows; | |
| // Ensure multiRows and multiCols remain integers after rounding. | |
| // Prefer keeping multiRows if it still evenly divides the new numCgras; | |
| // otherwise, fall back to a 1 × numCgras grid (which satisfies rows <= columns). | |
| if (numCgras % multiRows === 0) { | |
| multiCols = numCgras / multiRows; | |
| } else { | |
| multiRows = 1; | |
| multiCols = numCgras; | |
| } |
| # Clone VectorCGRA with submodules | ||
| RUN git clone --recursive https://github.com/tancheng/VectorCGRA.git |
There was a problem hiding this comment.
The Docker build uses an unpinned git clone of https://github.com/tancheng/VectorCGRA.git, pulling the default branch at build time, so any compromise or forced push on that repository can silently change what code is baked into this image and executed in your verification pipeline. Because this image is used to run VectorCGRA tooling, an attacker controlling that repo could inject arbitrary logic into the container environment or tamper with generated Verilog artifacts. Pin this dependency to an immutable identifier (e.g., a specific commit SHA or signed release) and avoid cloning a mutable branch head during the build.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 76 out of 127 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const isInFlight = status === 'pending' || status === 'running'; | ||
| // Synthesis is always allowed: if no SVerilog job has run yet, the executor | ||
| // falls back to the bundled default_design.v baked into the Docker image. | ||
| const canSynthesize = !!projectId && !isInFlight; | ||
|
|
||
| const handleSynthesize = useCallback(async () => { | ||
| if (!canSynthesize) return; | ||
|
|
||
| // Cancel any previous subscription | ||
| unsubscribeRef.current?.(); | ||
| unsubscribeRef.current = null; | ||
|
|
||
| setStatus('pending'); | ||
| setProgress(0); | ||
| setElapsedSeconds(0); | ||
| setResult(null); | ||
| setErrorMessage(null); | ||
| setStages([]); | ||
| stopTimer(); | ||
|
|
||
| let jobId; | ||
| try { | ||
| jobId = await submitSynthesisJob(projectId); | ||
| } catch (err) { | ||
| setStatus('error'); | ||
| setErrorMessage(err.message || 'Failed to submit synthesis job.'); | ||
| return; | ||
| } | ||
|
|
||
| setStatus('running'); | ||
| startTimer(); | ||
|
|
||
| const unsubscribe = subscribeToJob(jobId, ({ status: jobStatus, result: jobResult, error }) => { | ||
| if (jobStatus === 'running' || jobStatus === 'queued') { | ||
| // Update progress bar and accumulate stage log | ||
| if (jobResult?.progress != null) { | ||
| setProgress(jobResult.progress); | ||
| if (jobResult.stage) { | ||
| setStages((prev) => { | ||
| // Avoid duplicating the same progress entry | ||
| if (prev.length > 0 && prev[prev.length - 1].progress === jobResult.progress) { | ||
| return prev; | ||
| } | ||
| return [...prev, { | ||
| progress: jobResult.progress, | ||
| stage: jobResult.stage, | ||
| timeCost: jobResult.timeCost ?? null, | ||
| }]; | ||
| }); | ||
| } | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| if (jobStatus === 'success') { | ||
| unsubscribeRef.current?.(); | ||
| unsubscribeRef.current = null; | ||
| stopTimer(); | ||
| setProgress(100); | ||
| setResult(jobResult); | ||
| setStatus('done'); | ||
| return; | ||
| } | ||
|
|
||
| if (jobStatus === 'failed') { | ||
| unsubscribeRef.current?.(); | ||
| unsubscribeRef.current = null; | ||
| stopTimer(); | ||
| setStatus('error'); | ||
| setErrorMessage(error || 'Synthesis job failed.'); | ||
| } | ||
| }); | ||
|
|
||
| unsubscribeRef.current = unsubscribe; | ||
| }, [canSynthesize, projectId, startTimer, stopTimer]); | ||
|
|
||
| const synthesizeButton = ( | ||
| <Button | ||
| variant="contained" | ||
| fullWidth | ||
| disabled={!canSynthesize} | ||
| onClick={handleSynthesize} | ||
| startIcon={ | ||
| isInFlight | ||
| ? <CircularProgress size={16} color="inherit" /> | ||
| : <PlayArrowIcon /> | ||
| } | ||
| > | ||
| {isInFlight ? 'Synthesizing\u2026' : 'Synthesize'} | ||
| </Button> | ||
| ); | ||
|
|
||
| return ( | ||
| <Box sx={{ display: 'flex', flexDirection: 'column', gap: 1.5 }}> | ||
|
|
||
| {/* Synthesize button — tooltip hints about default design when SVerilog not yet generated */} | ||
| {!sverilogReady ? ( | ||
| <Tooltip | ||
| title="No SVerilog generated yet — will use the bundled default design for synthesis" | ||
| placement="top" | ||
| > | ||
| <span style={{ display: 'block' }}>{synthesizeButton}</span> | ||
| </Tooltip> | ||
| ) : ( | ||
| synthesizeButton |
There was a problem hiding this comment.
sverilogReady is only used to change the tooltip, but it does not actually gate submission: canSynthesize ignores sverilogReady, so the button remains enabled even when sverilogReady is false. If the intended contract is “disable until SVerilog is ready” (as implied by the prop name and VerificationTab comments), include sverilogReady in canSynthesize/disabled and adjust the tooltip text accordingly.
| //======================================================================== | ||
| // VCgraTemplateRTL_v.cpp | ||
| //======================================================================== | ||
| // This file provides a template for the C wrapper used in the import | ||
| // pass. The wrapper exposes a C interface to CFFI so that a | ||
| // Verilator-generated C++ model can be driven from Python. | ||
| // This templated is based on PyMTL v2. | ||
|
|
||
| #include "obj_dir_CgraTemplateRTL/VCgraTemplateRTL.h" | ||
| #include "stdio.h" | ||
| #include "stdint.h" | ||
| #include "verilated.h" | ||
| #include "verilated_vcd_c.h" | ||
|
|
There was a problem hiding this comment.
These appear to be generated Verilator/PyMTL wrapper artifacts (C++ wrapper, obj_dir outputs, etc.). Committing generated build outputs (especially those embedding absolute paths/tool versions) will bloat the repo and tends to cause noisy diffs. Prefer generating these at runtime and add the directory (e.g., test_vectorcgra/verilog/obj_dir_*, generated .mk/.dat/.cpp outputs) to .gitignore, keeping only the source scripts needed to reproduce the generation.
| RUN wget -q https://github.com/YosysHQ/oss-cad-suite-build/releases/download/2024-09-20/oss-cad-suite-linux-x64-20240920.tgz && \ | ||
| tar -C /cgra -xzf oss-cad-suite-linux-x64-20240920.tgz && \ | ||
| rm oss-cad-suite-linux-x64-20240920.tgz | ||
| ENV PATH="/cgra/oss-cad-suite/bin:${PATH}" | ||
|
|
||
| # Install Haskell Stack — required to compile sv2v from source | ||
| # Uses upstream installer script; stack binary is placed in /usr/local/bin | ||
| RUN curl -sSL https://get.haskellstack.org/ | sh -s -- -d /usr/local/bin | ||
|
|
||
| # Clone and build sv2v (SystemVerilog → Verilog converter) | ||
| # Pinned to commit 9bab0448 for reproducibility — same commit used in CI | ||
| RUN git clone https://github.com/zachjs/sv2v.git /cgra/sv2v && \ | ||
| cd /cgra/sv2v && \ | ||
| git checkout 9bab0448e32504cef764692018914f0f2f314911 && \ | ||
| stack setup && \ | ||
| make |
There was a problem hiding this comment.
The Dockerfile installs Stack via curl ... | sh and downloads oss-cad-suite without any checksum/signature verification. Piping remote scripts to sh and unauthenticated downloads are supply-chain risks and reduce build reproducibility. Prefer installing Stack from a pinned package/source with checksum verification, and verify the oss-cad-suite tarball (e.g., pinned SHA256) before extracting.
| * Low power: 1x1 (single CGRA) | ||
|
|
||
| ## Available FU Types (for tile configuration): | ||
| - Compute: add, mul, div, shift, logic | ||
| - Floating-point: fadd, fmul, fdiv, fmul_fadd, fadd_fadd, vfmul | ||
| - Control: cmp, sel, phi, loop_control | ||
| - Memory: mem, mem_indexed, constant | ||
| - Other: return, grant, alloca, type_conv | ||
| - Integer Arithmetic: alu (add/sub), mul, div, shift, mac | ||
| - Logic: logic (and/or/not/xor), cmp (icmp/fcmp), sel | ||
| - Floating-point: fadd, fmul, fdiv, fmul_fadd, fadd_fadd | ||
| - Memory: mem (load/store), mem_indexed, alloca, gep, memset | ||
| - Type: type_conv, constant | ||
| - Control: phi, return, branch, loop_control | ||
| - Data Movement: data_mov, ctrl_mov, reserve, data | ||
| - Grants: grant | ||
| - Vector: vadd, vmul, vfmul, vector | ||
|
|
||
| IMPORTANT: By default, all FU types should be enabled in every tile whenever we design a CGRA. | ||
| The full FU list is: ["add", "mul", "div", "fadd", "fmul", "fdiv", "logic", "cmp", "sel", "type_conv", "vfmul", "fadd_fadd", "fmul_fadd", "loop_control", "phi", "constant", "mem", "mem_indexed", "shift", "return", "alloca", "grant"] | ||
| The full FU list is: ["alu", "mul", "div", "shift", "mac", "logic", "cmp", "sel", "fadd", "fmul", "fdiv", "fmul_fadd", "fadd_fadd", "mem", "mem_indexed", "alloca", "gep", "memset", "type_conv", "constant", "phi", "return", "branch", "loop_control", "data_mov", "ctrl_mov", "reserve", "data", "grant", "vadd", "vmul", "vfmul", "vector"] | ||
| IMPORTANT: Use exactly these FU names. Do NOT use instruction names (e.g., use "alu" not "add", use "mem" not "load"). | ||
| For low_power designs, you may remove some FUs, but ALWAYS keep at minimum: alu, mul, cmp, sel, phi, constant, mem, gep, return, branch, data_mov, ctrl_mov, reserve, data, grant |
There was a problem hiding this comment.
PR title/description suggest a narrow FU-types mapping fix, but this PR also adds/changes verification UI, runner executors (run_tests/synthesis), Docker image toolchain installs, and checks in large generated Verilog/Verilator artifacts. Please update the PR title/description (or split into smaller PRs) so reviewers can scope risk and intent accurately.
| import { getAllFunctionUnits, getUnitForInstruction, FUNCTION_UNIT_TO_INSTRUCTIONS } from '../../shared/functionalUnitMapping.js'; | ||
|
|
There was a problem hiding this comment.
FUNCTION_UNIT_TO_INSTRUCTIONS is imported here but never used in this module. This will trip linting/CI in many setups and makes it harder to see what dependencies are actually required. Remove the unused import (or start using it).
| if (invalidEntries.length > 0) { | ||
| fixes.push(`Resolved/removed invalid FU types: ${invalidEntries.join(', ')}`); | ||
| } | ||
| fixedConfig.fu_types = validFus.length > 0 ? validFus : DEFAULT_FU_TYPES; | ||
| fixedConfig.fu_types = resolvedFus.size > 0 ? [...resolvedFus] : DEFAULT_FU_TYPES; |
There was a problem hiding this comment.
The fix message says "Resolved/removed invalid FU types" but it only lists entries that were not resolved (i.e., those pushed into invalidEntries). This is misleading when debugging AI config repairs. Consider splitting this into two lists (resolved instruction names vs removed invalid entries) or adjust the message to match what’s actually reported.
| # Clone VectorCGRA | ||
| RUN git clone https://github.com/tancheng/VectorCGRA.git | ||
| # Clone VectorCGRA with submodules | ||
| RUN git clone --recursive https://github.com/tancheng/VectorCGRA.git |
There was a problem hiding this comment.
The Dockerfile uses git clone --recursive https://github.com/tancheng/VectorCGRA.git without pinning to a specific commit or tag. Because this image is used to run untrusted mapping/synthesis jobs, a compromise or forced rewrite of the tancheng/VectorCGRA repository could inject arbitrary code into the build and runtime environment, leading to tampering with results or exfiltration of secrets. To mitigate this supply-chain risk, pin the clone to an immutable commit SHA (or vendor the code) and update explicitly when upgrading.
| ENV PATH="/cgra/sv2v/bin:${PATH}" | ||
|
|
||
| # Clone mflowgen (ASIC flow manager) — installed as a Python package after venv is set up below | ||
| RUN git clone https://github.com/tancheng/mflowgen.git /cgra/mflowgen |
There was a problem hiding this comment.
The Dockerfile clones https://github.com/tancheng/mflowgen.git without pinning to a specific commit or release, then installs it into the container with pip install -e. This means every image build pulls whatever code is currently on the default branch of a third-party repo, so a compromise or malicious update there could execute attacker-controlled code during builds or at runtime inside the synthesis flow. To reduce this supply-chain attack surface, pin mflowgen to a known-good commit SHA or tagged release, or vendor it into this repository.
Opus fixed mapping issue~