Fix cache set/tag indexing for non-power-of-two num_set and add regression coverage

[Copilot]

cache_c::find_tag_and_set() assumed num_set was power-of-two by using log2+mask indexing, which made some configured sets unreachable when num_set was not power-of-two. This change preserves legacy mapping for power-of-two configs and adds correct modulo/division mapping for all other positive set counts.

• Indexing logic (src/cache.cc, src/cache.h)

  • Unified set/tag derivation on line_number = index_addr >> m_shift_bits after existing tile/interleave address normalization.
  • Kept existing fast path for power-of-two set counts.
  • Added non-power-of-two path:
    • set = line_number % m_num_sets
    • tag = line_number / m_num_sets
  • Avoids masked-tag derivation on the non-power-of-two path.

• Input validation (src/cache.cc)

  • Added constructor guard to reject num_set <= 0 using existing assertion/error style (ASSERTM + abort path).

• Regression test (src/cache_set_indexing_test.cc, SConscript)

  • Added focused test target (cache_set_test=1) that verifies:
    • num_set=6 reaches all set indices with sequential cache-line addresses.
    • Distinct lines mapping to same set produce distinct tags.
    • num_set=8 retains legacy mask-based mapping.

• Documentation touch-up (src/cache.h)

  • Updated constructor param docstring to clarify num_set must be > 0 and is not restricted to power-of-two.

Addr line_number = index_addr >> m_shift_bits;
if (cache_sets_are_power_of_two(m_num_sets)) {
  *set = line_number & m_set_mask;
  *tag = line_number & m_tag_mask;
} else {
  *set = line_number % m_num_sets;
  *tag = line_number / m_num_sets;
}

• Validation commands and outcomes (required by issue)
  • ~/.local/bin/scons -j4 cache_set_test=1 .opt_build/cache_set_indexing_test ✅
  • ./.opt_build/cache_set_indexing_test ✅
  • ./build.py ✅
  • ./build.py -d ✅

[ejchung0406]

Please drop the test case from this PR. Concretely:

• Remove src/cache_set_indexing_test.cc and the cache_set_test block in SConscript.
• With the test gone there is no reason to expose cache_sets_are_power_of_two() and
  cache_line_to_set_and_tag() in cache.h. Fold that logic back into
  cache_c::find_tag_and_set() in cache.cc. That also removes the unused set_bits
  parameter.
• Change the set count check to the repo's usual one-line style:
  ASSERTM(m_num_sets > 0, "cache:%s num_set:%d\n", m_name.c_str(), m_num_sets);
  Drop the extra abort().

The set/tag logic itself looks correct, so keep that as is.

[ejchung0406]

@copilot, Please drop the test case from this PR. Concretely:

Remove src/cache_set_indexing_test.cc and the cache_set_test block in SConscript.
With the test gone there is no reason to expose cache_sets_are_power_of_two() and
cache_line_to_set_and_tag() in cache.h. Fold that logic back into
cache_c::find_tag_and_set() in cache.cc. That also removes the unused set_bits
parameter.
Change the set count check to the repo's usual one-line style:
ASSERTM(m_num_sets > 0, "cache:%s num_set:%d\n", m_name.c_str(), m_num_sets);
Drop the extra abort().
The set/tag logic itself looks correct, so keep that as is.

[Copilot]

@copilot, Please drop the test case from this PR. Concretely:

Remove src/cache_set_indexing_test.cc and the cache_set_test block in SConscript....

Addressed in e76bb4b.

• Removed src/cache_set_indexing_test.cc
• Removed the cache_set_test block from SConscript
• Folded set/tag mapping logic into cache_c::find_tag_and_set() in src/cache.cc
• Removed helper exposure from src/cache.h
• Switched set-count validation to ASSERTM(m_num_sets > 0, ...) and removed abort()

[ejchung0406]

@saursin can u approve the review? :)

[Copilot]

🟡 Changes recommended

The PR description claims new regression test code/target, but the corresponding test source/build wiring is not present in the actual changes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates cache_c::find_tag_and_set() so caches with a non-power-of-two num_set can reach all sets by switching to modulo/division-based indexing when appropriate, while preserving the legacy mask-based mapping for power-of-two configurations.

Changes:

• Refactors cache_c::find_tag_and_set() to compute line_number after tile/interleave normalization and derives (set, tag) via mask or modulo/division depending on whether m_num_sets is a power of two.
• Adds a constructor assertion to require num_set > 0.
• Updates the constructor parameter docstring for num_set; trims a trailing blank line in SConscript.

File summaries

File	Description
src/cache.cc	Adds num_set > 0 guard and updates set/tag derivation to support non-power-of-two set counts.
src/cache.h	Clarifies num_set parameter meaning/constraints in the constructor docstring.
SConscript	Minor whitespace-only change at end of file.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Updated the PR description to remove the references to the regression test target/file, since that wasn't part of this change.