Add enhanced branch predictor phase-1 integration and tests by kumavatkomal · Pull Request #301 · riscv-software-src/riscv-perf-model

kumavatkomal · 2026-04-20T15:59:54Z

This is a PR for Phase-1 branch predictor integration work.
The goal is to land implementation and test wiring first, while keeping EDM comparison and final performance claims for follow-up work.

Included in this PR

Enhanced branch predictor integration in fetch flow
Predictor selection and config wiring
Branch prediction evaluation and update path integration
Branch predictor unit and integration test updates
Required architecture parameter updates for predictor selection

Not included yet

Final EDM vs trace comparison results
Final measured claim locking based on EDM runs
Final report artifacts and final comparison tables

Validation completed

Build completed for updated targets
Branch predictor tests passed after integration updates

Commands used for validation

cmake --build build --target fetch BranchPred_test -j4
./build/test/core/branch_pred/BranchPred_test

issue tracking

Related to #1

bdutro · 2026-05-04T14:27:33Z

+        std::transform(branch_predictor_name_.begin(), branch_predictor_name_.end(),
+                       branch_predictor_name_.begin(),
+                       [](unsigned char ch) { return static_cast<char>(std::tolower(ch)); });


std::transform shouldn't be used for in-place modification of a container:

std::transform does not guarantee in-order application of unary_op or binary_op. To apply a function to a sequence in-order or to apply a function that modifies the elements of a sequence, use std::for_each.

bdutro · 2026-05-04T14:28:36Z

+        const auto is_power_of_two = [](const uint32_t value) {
+            return value && ((value & (value - 1)) == 0);
+        };


Use sparta::utils::is_power_of_2

bdutro · 2026-05-04T14:32:41Z

+    bool isPowerOfTwo(const uint32_t value)
+    {
+        return value && ((value & (value - 1)) == 0);
+    }


Use sparta::utils::is_power_of_2

bdutro · 2026-05-04T14:40:38Z

+    uint64_t EnhancedBranchPredictor::btbCacheAddress_(uint64_t fetch_pc) const {
+        // Convert instruction-addressed fetch PC into the cache-line address
+        // expected by SimpleCache2 while preserving intended BTB indexing.
+        return fetch_pc << (kBTBCacheLineShift - kCompressedIndexShift);


I think you want this to be a right-shift?

Is this still an issue?

Oh, good call. @kumavatkomal this should be a right shift.

Right shift causes 6 test failures in BranchPred_test. Left shift passes all tests. Should the tests be updated, or am I missing something else that needs to change?

Ok, I understand what's happening now. The Sparta cache for the BTB is configured with 1KB lines, so the cache address decoder does >> 10 to get the index bits. We actually want it to use PC[8:1] for the lookup, so left shifting it by 9 bits first takes care of that.

A different way to handle this would be to set kBTBCacheLineBytes = 2 and configure btb_cache_ like so:

btb_cache_ = std::make_unique<sparta::cache::SimpleCache2<BTBCacheLine>>( btb_entries_ * kBTBCacheLineBytes, kBTBCacheLineBytes, kBTBCacheLineBytes, BTBCacheLine(), *btb_replacement_policy_, false); }

This would configure the address decoder in the cache to just >> 1, and then this function can be removed.

Implemented your suggestion:

kBTBCacheLineBytes = 2 with btb_entries_ * kBTBCacheLineBytes and false parameter

Removed btbCacheAddress_() function, now using fetch_pc directly

Cache decoder now does >> 1 for PC[8:1] indexing

All tests pass. Is this correct?

bdutro · 2026-05-04T14:42:19Z

+    bool EnhancedBranchPredictor::btbLookup_(uint64_t fetch_pc, BTBEntry * entry) {
+        auto cache_line = btb_cache_->peekLine(btbCacheAddress_(fetch_pc));
+        if ((cache_line != nullptr) && cache_line->isValid()) {
+            if (entry != nullptr) {
+                *entry = cache_line->btb_entry;
+            }
+            // Keep replacement state hot for recently used BTB lines.
+            btb_cache_->touchMRU(*cache_line);
+            return true;
+        }
+        return false;
+    }


You could just have this return a pointer to cache_line->btb_entry on a hit and a nullptr on a miss. This would also avoid the overhead of copying around a BTBEntry on every hit.

bdutro · 2026-05-04T14:46:28Z

+        // One path for both invalid-line fill and victim replacement.
+        auto & replacement_line = btb_cache_->getLineForReplacementWithInvalidCheck(btb_addr);
+        btb_cache_->allocateWithMRUUpdate(replacement_line, btb_addr);
+        replacement_line.btb_entry = entry;


Consider altering replacement_line.btb_entry directly instead of copying over it.

bdutro · 2026-05-04T14:52:24Z

+        } else {
+            const uint64_t default_target = update.actually_taken ?
+                update.corrected_PC :
+                (update.fetch_PC + update.branch_idx + bytes_per_inst);
+            entry = BTBEntry(update.branch_idx, default_target);
+        }


One potential optimization here is to only allocate new BTB entries after a taken branch is mispredicted.

bdutro · 2026-05-04T15:01:26Z

+        for (auto it = insts_to_send->begin(); it != insts_to_send->end(); ++it, ++i)
+        {
+            const auto & inst = *it;
+            if (!inst->isBranch()) {
+                continue;
+            }
+
+            found_branch = true;
+            actual_branch_idx = i;
+            actual_branch_inst = inst;
+            if (inst->isTakenBranch()) {
+                actual_next_pc = inst->getTargetVAddr();
+            } else {
+                actual_next_pc = inst->getPC() + BranchPredictorIFType::bytes_per_inst;
+            }
+            break;
+        }


Alternatively, you could do something like this:

auto it = std::find_if(insts_to_send->begin(), insts_to_send->end(), [](const InstPtr& inst) { return inst->isBranch(); }); if(it == insts_to_send->end()) { return; } // Now process the branch update

bdutro · 2026-05-04T15:02:53Z

+        InstPtr actual_branch_inst;
+
+        // Predictor contract: one prediction per fetch packet, keyed by fetch PC.
+        // We therefore train/evaluate against the first branch in this packet.


What if there are multiple branches in a fetch packet? If there is a never taken conditional branch followed by an always taken conditional branch, the second branch will never be inserted into the BTB and we will always mispredict.

bdutro · 2026-05-04T15:08:50Z

+            if (bhtPredict_(input.fetch_PC)) {
+                prediction.predicted_PC = btb_entry.predicted_PC;
+            } else {
+                prediction.predicted_PC = input.fetch_PC + prediction.branch_idx + bytes_per_inst;


This isn't the correct fallthrough PC. The way branch_idx is calculated in Fetch::evaluateBranchPrediction_ doesn't account for non-branch instructions in a fetch bundle.

klingaard

Love the testing!

klingaard · 2026-05-30T13:34:14Z


 top.cpu.core0:
  fetch.params.num_to_fetch:   3
+  fetch.params.branch_predictor: simple


Since medium core inherits from small core, it automatically picks up the simple branch predictor.

klingaard · 2026-05-30T13:34:37Z


 top.cpu.core0:
  fetch.params.num_to_fetch:   8
+  fetch.params.branch_predictor: simple


Since big core inherits from medium/small core, it automatically picks up the simple branch predictor.

klingaard · 2026-05-30T13:35:51Z

+    }
+
+    uint32_t EnhancedBranchPredictor::bhtIndex_(uint64_t fetch_pc) const {
+        return static_cast<uint32_t>((fetch_pc >> 1) & (bht_entries_ - 1));


Won't this lose bit precision?

The cast happens after masking, so precision is preserved. The mask (bht_entries_ - 1) extracts only the lower 9 bits (for 512 entries), which easily fits in uint32_t. No bits are lost.

Is there a specific precision concern I should address?

Since this goes directly into a vector lookup, you could return a size_t instead and avoid a conversion.

klingaard · 2026-05-30T13:37:57Z

+    }
+
+    bool EnhancedBranchPredictor::bhtPredict_(uint64_t fetch_pc) const {
+        return branch_history_table_[bhtIndex_(fetch_pc)] > 1;


Suggest you use at to avoid overflow: branch_history_table_.at()

klingaard · 2026-05-30T13:39:27Z

+    }
+
+    void EnhancedBranchPredictor::bhtUpdate_(uint64_t fetch_pc, bool actually_taken) {
+        auto & counter = branch_history_table_[bhtIndex_(fetch_pc)];


klingaard · 2026-05-30T13:44:45Z

+        branch_predictor_name_(p->branch_predictor),
+        enhanced_btb_entries_(p->enhanced_btb_entries),
+        enhanced_btb_ways_(p->enhanced_btb_ways),
+        enhanced_bht_entries_(p->enhanced_bht_entries),


Why are these members of Fetch? Does Fetch need to know this information? Looks like it's passed to the predictor instantiations below.

You're right - Fetch doesn't need to store these after construction. They're only used to pass to the predictor. Should I update this to read from ParameterSet and pass directly without storing as member variables?

klingaard · 2026-05-30T13:45:24Z

+            sparta_assert(enhanced_btb_entries_ > 0, "enhanced_btb_entries must be > 0");
+            sparta_assert(enhanced_btb_ways_ > 0, "enhanced_btb_ways must be > 0");
+            sparta_assert(enhanced_bht_entries_ > 0, "enhanced_bht_entries must be > 0");
+            sparta_assert((enhanced_btb_entries_ % enhanced_btb_ways_) == 0,
+                          "enhanced_btb_entries must be divisible by enhanced_btb_ways");
+            sparta_assert(sparta::utils::is_power_of_2(enhanced_btb_entries_ / enhanced_btb_ways_),
+                          "enhanced BTB set count must be power-of-two");
+            sparta_assert(sparta::utils::is_power_of_2(enhanced_bht_entries_),
+                          "enhanced_bht_entries must be power-of-two");


Should these asserts be in EnhancedBranchPredictor?

klingaard · 2026-05-30T13:47:11Z

+        BranchPredictor::DefaultUpdate update;
+        update.fetch_PC = input.fetch_PC;
+        update.branch_idx = actual_branch_idx;
+        update.corrected_PC = actual_next_pc;
+        update.actually_taken = actual_branch_inst->isTakenBranch();
+        branch_predictor_->updatePredictor(update);


I think the updatePredictor method should populate predictor object. Fetch should know nothing about how the predictor works.

Fetch builds DefaultUpdate because it has the instruction data. This is the same pattern used by SimpleBranchPredictor - both receive DefaultUpdate objects. Should I change both predictors to use a different approach, or is the current design acceptable?

No, let's keep the current interface for now. In the future we may want the update object to contain the instruction data.

klingaard · 2026-05-30T13:48:55Z

+
+        // Track end-to-end prediction quality at fetch integration level.
+        ++branch_predictions_;
+        if (mispredicted) {


@bdutro question for you: should we count the mispredict here or when the branch is resolved?

We should count them when the branch is resolved to avoid overcounting mispredicts that happen in the shadow of another mispredict/flush/etc.

klingaard · 2026-05-30T13:49:35Z

+        std::string branch_predictor_name_;
+
+        // Geometry knobs consumed when enhanced predictor is selected.
+        const uint32_t enhanced_btb_entries_;
+        const uint32_t enhanced_btb_ways_;
+        const uint32_t enhanced_bht_entries_;


Don't see a need to keep these around

Fixed - removed unused counter variables.

For the config variables (enhanced_btb_entries_, enhanced_btb_ways_, enhanced_bht_entries_): these are currently stored because they're read from ParameterSet and passed to predictor constructor. Should I update this to read and pass directly without storing? Let me know and I'll make the change.

Should I update this to read and pass directly without storing?

Yes, that's the preferred way to handle this.

bdutro · 2026-06-09T14:58:52Z

+                p->enhanced_btb_entries,
+                p->enhanced_btb_ways,
+                p->enhanced_bht_entries));


@kumavatkomal, you need to call ignore() on these parameters when branch_predictor_name_ != "enhanced" to fix the regression failures.

bdutro · 2026-06-09T15:00:52Z


+        std::for_each(branch_predictor_name_.begin(), branch_predictor_name_.end(),
+                      [](char& ch) { ch = static_cast<char>(std::tolower(static_cast<unsigned char>(ch))); });
+


Alternatively, you can do this here and then use the local variables when you instantiate the EnhancedBranchPredictor:

Suggested change

const auto enhanced_btb_entries = p->enhanced_btb_entries;

const auto enhanced_btb_ways = p->enhanced_btb_ways;

const auto enhanced_bht_entries = p->enhanced_bht_entries;

oluwatimilehin · 2026-09-02T23:47:12Z

Hi all, is there any more work planned for this PR, or is it just waiting to be merged?

Add enhanced branch predictor phase-1 integration and tests

de11c7f

kumavatkomal marked this pull request as ready for review May 4, 2026 13:35

bdutro reviewed May 4, 2026

View reviewed changes

Address review feedback for PR riscv-software-src#301

132ef43

klingaard reviewed May 30, 2026

View reviewed changes

Address PR riscv-software-src#301 review feedback round 2

3355135

bdutro reviewed Jun 9, 2026

View reviewed changes

Fix parameter reading for regression tests

ed9f154

bdutro approved these changes Jun 15, 2026

View reviewed changes


		std::for_each(branch_predictor_name_.begin(), branch_predictor_name_.end(),
		[](char& ch) { ch = static_cast<char>(std::tolower(static_cast<unsigned char>(ch))); });

+const auto enhanced_btb_entries = p->enhanced_btb_entries;
+const auto enhanced_btb_ways = p->enhanced_btb_ways;
+const auto enhanced_bht_entries = p->enhanced_bht_entries;

Uh oh!

Conversation

kumavatkomal commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klingaard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdutro Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumavatkomal commented Apr 20, 2026 •

edited

Loading

bdutro Jun 2, 2026 •

edited

Loading