Skip to content

Conversation

@n0thingNoob
Copy link
Contributor

  1. Update a script to generate the utilization graph
  2. Fix the store instruction (src1 is data, src2 is addr)
  3. Add histogram main.go

Copilot AI review requested due to automatic review settings January 20, 2026 01:24
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request introduces a utilization visualization tool, adds a histogram test benchmark, and attempts to fix the operand order for the StoreDirect instruction. However, there is a critical breaking change that will cause existing tests to fail.

Changes:

  • Added Python script tool/utilization_plot.py to generate tile and port utilization heatmaps from JSON logs
  • Added new histogram testbench in test/testbench/histogram/main.go with input data preloading and validation
  • Modified core/emu.go to swap StoreDirect operand order and removed extraneous whitespace

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
tool/utilization_plot.py New visualization tool that parses JSON logs and generates utilization heatmaps for CGRA tiles and ports
test/testbench/histogram/main.go New histogram test benchmark that preloads data, runs simulation, and validates output against expected results
core/emu.go Swaps StoreDirect operand order (src1=data, src2=address) and removes blank lines for cleaner formatting

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 90 to 92
scanLimit := 64
outputData := make([]uint32, scanLimit)
for addr := 0; addr < 5; addr++ {
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

The scanLimit variable is set to 64 but only 5 histogram bins are read from memory. This allocates more memory than needed. Consider setting scanLimit to 5 or using the bins parameter to match the actual number of histogram bins being read.

Suggested change
scanLimit := 64
outputData := make([]uint32, scanLimit)
for addr := 0; addr < 5; addr++ {
scanLimit := 5
outputData := make([]uint32, scanLimit)
for addr := 0; addr < scanLimit; addr++ {

Copilot uses AI. Check for mistakes.
Comment on lines 665 to +668
src1 := inst.SrcOperands.Operands[0]
addrStruct := i.readOperand(src1, state)
addr := addrStruct.First()
src2 := inst.SrcOperands.Operands[1]
valueStruct := i.readOperand(src2, state)
valueStruct := i.readOperand(src1, state)
value := valueStruct.First()
src2 := inst.SrcOperands.Operands[1]
addrStruct := i.readOperand(src2, state)
addr := addrStruct.First()
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

The operand order change for StoreDirect breaks existing test files. The test YAML files (e.g., test/mem/test_loadstore.yaml) have STD instructions with src_operands[0] as the address and src_operands[1] as the data value. However, after this change, the code reads src_operands[0] as data and src_operands[1] as address, which is the opposite. Either the fix needs to be reverted, or all existing YAML test files need to be updated to match the new operand order. The PR description states "src1 is data, src2 is addr" but the existing convention in YAML files is the opposite.

Copilot uses AI. Check for mistakes.
@n0thingNoob n0thingNoob requested a review from Jackcuii January 21, 2026 21:39
Copy link
Collaborator

@Jackcuii Jackcuii left a comment

Choose a reason for hiding this comment

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

LGTM

@Jackcuii Jackcuii merged commit 315c41d into main Jan 21, 2026
5 checks passed
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.

3 participants