Skip to content

FIXES ISSUE #473 : lowest/highest_set_bit functions should return a Bits type? #906

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

omsuneri
Copy link

This PR fixes #473.
The lowest_set_bit and highest_set_bit functions were returning XReg (i.e., Bits) though they only compute a position index. Updated to return Bits<8> instead, which is sufficient and semantically correct.
All smoke tests and IDL validations pass successfully.
@ThinkOpenly please review this and request if any further chnaged are required !!

@dhower-qc
Copy link
Collaborator

One thing to check here:

If the return of these functions is fed into an operator that might lose information from insufficient bit width, this could introduce a subtle bug. For example:

Bits<8> a = 128;
XReg b = a << 1;   # b = 0

XReg c = 128;
XReg d = c << 1;   # d = 256
---

Can you check how the returns are currently being used?

Copy link

codecov bot commented Jul 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.30%. Comparing base (77d4edc) to head (51065af).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #906   +/-   ##
=======================================
  Coverage   43.30%   43.30%           
=======================================
  Files          10       10           
  Lines        4787     4787           
  Branches     1298     1298           
=======================================
  Hits         2073     2073           
  Misses       2714     2714           
Flag Coverage Δ
idlc 43.30% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@omsuneri
Copy link
Author

@dhower-qc I tested the overflow behavior using tests/dev/test_shift_overflow.py

def simulate_bits8_shift():
    bits8 = 128
    result = (bits8 << 1) & 0xFF
    return result

def simulate_xreg_shift():
    xreg = 128
    result = xreg << 1 
    return result

def test_shift_behavior():
    bits8_result = simulate_bits8_shift()
    xreg_result = simulate_xreg_shift()

    print(f"Bits<8>: 128 << 1 = {bits8_result} (Expected: 0 if overflowed)")
    print(f"XReg   : 128 << 1 = {xreg_result} (Expected: 256)")

    assert bits8_result == 0, "Bits<8> incorrectly retained the value (should overflow)"
    assert xreg_result == 256, "XReg did not shift correctly"

if __name__ == "__main__":
    test_shift_behavior()

result:

root@08ff66764da5:/workspaces/riscv-unified-db# python3 tests/dev/test_shift_overflow.py
Bits<8>: 128 << 1 = 0 (Expected: 0 if overflowed)
XReg   : 128 << 1 = 256 (Expected: 256)

This confirms that using Bits<8> in return from lowest_set_bit / highest_set_bit may be unsafe unless all downstream uses are known to be non-shift, non-multiply....
what would you suggest me to do next !!

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.

lowest/highest_set_bit functions should return a Bits type?
2 participants