Skip to content

Conversation

@zhangkanqi
Copy link

No description provided.

Copilot AI review requested due to automatic review settings January 24, 2026 08:18
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.

Pull request overview

Updates instruction-extension dictionary keys in the instruction format table to use uppercase identifiers.

Changes:

  • Renamed INSTRUCTION_FORMATS top-level keys for rv_zifencei, rv_zbs, and rv_32B to uppercase equivalents.
  • Aligns these specific extension identifiers with the uppercase pattern used by many other entries.

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

},
# rv32
"rv_32B":{
"RV_32B":{
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

Changing this extension key to RV_32B can break lookups from callers/config that still use rv_32B (see asm_template_manager/ext_list/allowed_ext.py). Some code paths index INSTRUCTION_FORMATS using the extension value as-is, so rv_32B will no longer match and these instructions can be skipped. Please ensure extension names are normalized consistently (or add an alias from the old key).

Copilot uses AI. Check for mistakes.

INSTRUCTION_FORMATS = {
"rv_zifencei": {
"RV_ZIFENCEI": {
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

These extension keys were changed to uppercase, but several call sites and configuration lists still use the lowercase forms (e.g., fuzzer/generator/asm_template_manager/ext_list/allowed_ext.py contains "rv_zifencei"). Some code (e.g., core/generator/generate_instrs.py) indexes INSTRUCTION_FORMATS using the extension string without normalizing case, so rv_zifencei will no longer be found and the extension’s instructions may be silently skipped. Please standardize extension identifiers across the repo (update allowed_ext/special_probabilities/sets keys as needed) or add a backwards-compatible alias / normalization layer (e.g., normalize ext = ext.upper() before lookup).

Copilot uses AI. Check for mistakes.
},
# rv32
"rv_zbs": {
"RV_ZBS": {
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

This key rename to uppercase needs to be kept consistent with the extension strings used elsewhere (e.g., allowed_ext.py / special_probabilities.py currently include "rv_zbs"). Since some generation paths use the raw extension string for INSTRUCTION_FORMATS[ext] (without .upper()), this change can prevent Zbs instructions from ever being selected/generated. Please update all extension identifiers or normalize/alias extension names at lookup time.

Copilot uses AI. Check for mistakes.
@youzi27
Copy link
Contributor

youzi27 commented Jan 26, 2026

Thanks for the PR!

Please refer to the Copilot feedback above. Before merging, the backward compatibility issues mentioned there need to be addressed. Once those points are resolved, we can move forward with the merge.

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