Skip to content

Pe 1b rv#150

Merged
mcoduoza merged 4 commits intomapping_to_cgrafrom
PE_1b_rv
Apr 8, 2025
Merged

Pe 1b rv#150
mcoduoza merged 4 commits intomapping_to_cgrafrom
PE_1b_rv

Conversation

@mcoduoza
Copy link
Collaborator

@mcoduoza mcoduoza commented Apr 8, 2025

Corresponding AHA CI is here: StanfordAHA/aha#2023

@mcoduoza mcoduoza requested review from Copilot and mbstrange2 April 8, 2025 03:13
Copy link

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.

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

sam/onyx/hw_nodes/compute_node.py:238

  • [nitpick] The returned tuple now includes additional configuration values. Consider adding an inline comment or updating the method documentation to clarify the order and correspondence of these values with the configuration dictionary.
return (op_code, bypass_rv, pe_only, pe_in_external, active_inputs, 0, 1, 0), cfg_kwargs

sam/onyx/hw_nodes/compute_node.py:236

  • [nitpick] The inline comment for 'active_1b_output' is nearly identical to that for 'active_16b_output' and may cause confusion. Consider rewording the comment to clearly distinguish the intended behavior for the 1b output.
'active_1b_output': 0  # sparse apps can only use the 16b output of the ALU and cannot use the 1b output

Copy link
Collaborator

@mbstrange2 mbstrange2 left a comment

Choose a reason for hiding this comment

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

Could you make the 0, 1, 0 be variables instead that are hardcoded to match the style of the rest of this? Will make future changes more seamless if needed.

@mcoduoza
Copy link
Collaborator Author

mcoduoza commented Apr 8, 2025

Could you make the 0, 1, 0 be variables instead that are hardcoded to match the style of the rest of this? Will make future changes more seamless if needed.

Sure! done

Copy link
Collaborator

@mbstrange2 mbstrange2 left a comment

Choose a reason for hiding this comment

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

LGTM

@mcoduoza mcoduoza merged commit e0967d8 into mapping_to_cgra Apr 8, 2025
0 of 2 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