Skip to content

Conversation

Sukuna0007Abhi
Copy link
Contributor

Fixes #1061: Can't describe instructions with overlapping match strings

Changes

1. Fix decode tree crash (decode_tree.rb)

  • Problem: inst_format.reverse[cur_range] can return nil, causing match? to crash
  • Solution: Added nil safety check with range_bits && range_bits.match?
  • Why needed: Without this, the decode tree crashes before it can process any overlapping patterns

2. Add missing Zicfilp instruction definitions

  • Problem: Sir @ThinkOpenly identified that sspush/sspopchk were "represented with incorrect mnemonics"
  • Solution: Created proper instruction definitions with constraint-based disambiguation
  • Files added:
    • sspush.yaml - Variable form with not constraints (allows only x1/x5)
    • sspush.x1.yaml, sspush.x5.yaml - Fixed forms for specific registers
    • sspopchk.yaml - Variable form with not constraints (allows only x1/x5)
    • sspopchk.x1.yaml, sspopchk.x5.yaml - Fixed forms for specific registers
  • Why this works: The patterns use constraint-based disambiguation where not constraints make seemingly overlapping patterns distinguishable

3. Fix Rakefile build issue

  • Problem: Missing require "pathname" causing build failures
  • Solution: Added the missing require statement
  • Why needed: Build system needs pathname support to process the new instruction files

Ready for Review sir @ThinkOpenly @dhower-qc

…tions

Fixes riscv-software-src#1061: Can't describe instructions with overlapping match strings

- Add nil safety check in decode_tree.rb to prevent NoMethodError
  when inst_format.reverse[cur_range] returns nil
- Create missing Zicfilp instruction definitions:
  - sspush.yaml, sspush.x1.yaml, sspush.x5.yaml
  - sspopchk.yaml, sspopchk.x1.yaml, sspopchk.x5.yaml
- Fix Rakefile missing 'require pathname' statement
- Instructions are naturally distinguishable by bit patterns,
  no overlapping constraints needed

Signed-off-by: Sukuna0007Abhi <[email protected]>
@Sukuna0007Abhi Sukuna0007Abhi changed the title fix : decode tree crash with nil safety and add missing Zicfilp instructions fix: decode tree crash with nil safety and add missing Zicfilp instructions Sep 4, 2025
Copy link
Collaborator

@dhower-qc dhower-qc left a comment

Choose a reason for hiding this comment

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

A few high-level comments:

  • Good job. That decode_tree code is pretty hard to understand. I wrote it and can barely parse it ;)
  • This should be two PRs; one for the decode_tree fix and one to add the instructions.
  • CI fails because you haven't identified the new instructions as "hints" of mops. You'll see an example of how to do it in the mop files.

@Sukuna0007Abhi
Copy link
Contributor Author

Sukuna0007Abhi commented Sep 4, 2025

A few high-level comments:

  • Good job. That decode_tree code is pretty hard to understand. I wrote it and can barely parse it ;)

Thanks sir @dhower-qc , Yes sir it's a pretty hard and a bit confusing but I will try my best ,

  • This should be two PRs; one for the decode_tree fix and one to add the instructions.

yes sir I will create 2 PR's,

  • CI fails because you haven't identified the new instructions as "hints" of mops. You'll see an example of how to do it in the mop files.

Yes sir I found it now, and also there some wrong in not constraint I think.

@Sukuna0007Abhi
Copy link
Contributor Author

Sukuna0007Abhi commented Sep 4, 2025

I am pointing this PR to add the instructions only

- Change not constraints from long exclusion list to only exclude registers 1 and 5
- This allows the general instructions to work with any register except x1 and x5
- The specific variants (sspush.x1, sspush.x5, sspopchk.x1, sspopchk.x5) handle those exact cases
- Resolves encoding conflicts between general and specific instruction variants

Fixes riscv-software-src#1061

Signed-off-by: Sukuna0007Abhi <[email protected]>
inst_format = inst.encoding(xlen).format
if inst_format.reverse[cur_range].match?(/^[01]+$/)
range_bits = inst_format.reverse[cur_range]
if range_bits && range_bits.match?(/^[01]+$/)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this just converts a crash into an infinite loop. That's what I'm seeing in testing, anyway. Are you seeing something different?

Copy link
Contributor Author

@Sukuna0007Abhi Sukuna0007Abhi Sep 4, 2025

Choose a reason for hiding this comment

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

Oops sorry sir @ThinkOpenly I think I missed to add the another needy few lines here, sorry it's because I got very sad after lfx result😞 , sure sir will fix that

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.

Can't describe instructions with overlapping match strings discriminated by "not" constraints

3 participants