Question about the Indirect Predictor Refactor

[OdnetninI]

Hi!
I was working on the indirect predictor and I found something weird regarding the commit: "cpu: Refactor indirect predictor " ( f65df9b ).

The indirect predictor is checked for all indirect branches that are not function returns:

        // The target address is not predicted by RAS.
        // Thus, BTB/IndirectBranch Predictor is employed.
        if (!inst->isReturn()) {
            if (inst->isDirectCtrl() || !iPred) {

After doing the prediction, the indirect predictor is used to perform a recordIndirect to save the prediction into the history.

f65df9b#diff-6b913cefadced10d4eefae514686d1bedb4709dc4896cfef0a1b3e5fab3b5a4eL276

This functionality was moved to the new update function, also executed after the prediction:

Old Code:

gem5/src/cpu/pred/simple_indirect.cc

Lines 120 to 126 in aa949db

	SimpleIndirectPredictor::recordIndirect(Addr br_addr, Addr tgt_addr,
	InstSeqNum seq_num, ThreadID tid)
	{
	DPRINTF(Indirect, "Recording %x seq:%d\n", br_addr, seq_num);
	HistoryEntry entry(br_addr, tgt_addr, seq_num);
	threadInfo[tid].pathHist.push_back(entry);
	}

New Code:

gem5/src/cpu/pred/simple_indirect.cc

Lines 240 to 248 in 7d62da6

	// Only indirect branches are recorded in the path history
	if (history->was_indirect) {
	DPRINTF(Indirect, "Recording %x seq:%d\n", history->pcAddr, sn);
	threadInfo[tid].pathHist.emplace_back(
	history->pcAddr, target.instAddr(), sn);
	stats.indirectRecords++;
	}

As you can see, in the new code, this update is protected under the history->was_indirect flag.
The flag is set to true every time a prediction is done by the indirect predictor, but it is updated in the update function ( https://github.com/gem5/gem5/blob/7d62da6d10fe54b9a9967e4eaae9554a75990c06/src/cpu/pred/simple_indirect.cc#L218C5-L218C57 ).

The function isIndirectNoReturn was added in the refactor commit: f65df9b#diff-ed549c292364838d1a0f401121baf9b4e3ac755cde44fda070214d9a802e7ec4R161-R164

    inline bool isIndirectNoReturn(BranchType type) {
        return (type == BranchType::CallIndirect) ||
               (type == BranchType::IndirectUncond);
    }

Is it missing the Indirect Conditional branches?

Previous to the commit, the only checks were:

• Is not a Return
• Is not a Direct Branch

In the pull request ( #429 ) there is a comment confirming this new version has the exactly same results as the previous version ( #429 (comment) ).
This makes sense in x86 because there are no Indirect Conditional branches.

[giactra]

@dhschall Yes, it looks like a bug. There is no Indirect Conditional branch in Arm neither. Regarding other ISAs, in gem5 only PowerPC seems to implement them https://github.com/gem5/gem5/blob/stable/src/arch/power/isa/formats/branch.isa#L155.

Feel free to open a PR if you want to fix this