c.jr/c.jalr semantic wrong

[Ye-Jinhong]

Describe the bug
As RISC-V unpriviledge isa spec says, c.jr/c.jalr expands to jalr x0/x1 0(rs1). Since jalr should force the bit 0 to zero, I think c.jr/c.jalr should act the same way.

[hnpl]

Hi @Ye-Jinhong, can you elaborate the issue more? (i.e., what was the unexpected output for c.jal/c.jalr?)

[Ye-Jinhong]

Hi @hnpl , it is specified that jalr should set the bit 0 to zero in RISC-V unpriviledge isa spec.

The indirect jump instruction JALR (jump and link register) uses the I-type encoding. The target address is obtained by adding the sign-extended 12-bit I-immediate to the register rs1, then setting the least-significant bit of the result to zero.

And in "C" extension, c.jr/c.jalr expands to jalr x0/x1 0(rs1)

C.JR (jump register) performs an unconditional control transfer to the address in register rs1. C.JR expands to jalr x0, 0(rs1).

C.JALR (jump and link register) performs the same operation as C.JR, but additionally writes the address of the instruction following the jump (pc+2) to the link register, x1. C.JALR expands to jalr x1, 0(rs1).

So the result of c.jr/c.jalr rs1 which is the next fetch PC should equals to 0 + rs1 of jalr, setting the bit 0 to zero. This means the result is the value (rs1 + 0) & ~0x1.

In decoder.isa, line 450 to 456

                0x0: Jump::c_jr({{
                    if (RC1 == 0) {
                        return std::make_shared<IllegalInstFault>(
                                "source reg x0", machInst);
                    }
                    NPC = rvZext(Rc1);
                }}, IsIndirectControl, IsUncondControl);

and line 468 to 471

                    default: Jump::c_jalr({{
                        ra = rvSext(NPC);
                        NPC = rvZext(Rc1);
                    }}, IsIndirectControl, IsUncondControl, IsCall);

The NPC is not correct due to not masking the bit 0 like NPC = rvZext(Rc1) & ~0x1.

I came across this when I build my own o3 cpu model, when the rs1 is not even in wrong path, fetch unit will fetch from the unaligned Addr. Although this will not cause error as a squash following, but it is a sematic wrong against RISC-V unpriviledge spec.

Affects version
All version.

Expected behavior
The next PC should always be even no matter it is on right instruction path or not.

[hnpl]

I see. If I'm not mistaken, that's a bug.

Interestingly, gem5 O3CPU does jump to the correct location due to branch prediction mechanism [1]. This is because c_jr's and c_jalr's branchTarget() function do generate the correct address, and if Rc1 is not 2-byte aligned, it is a branch misprediction (and if the branch predictor learned this, there'll be no misprediction later on).

I'm not sure about the c_jr and c_jalr in other gem5 cpu types given that they call the instructions's execute() function directly to get the new PC.

    std::unique_ptr<PCStateBase>
    C_jr::branchTarget(ThreadContext *tc) const
    {
        PCStateBase *pc_ptr = tc->pcState().clone();
        pc_ptr->as<PCState>().set((tc->getReg(srcRegIdx(0)) + imm) & ~0x1);
        return std::unique_ptr<PCStateBase>{pc_ptr};
    }

    std::unique_ptr<PCStateBase>
    C_jalr::branchTarget(ThreadContext *tc) const
    {
        PCStateBase *pc_ptr = tc->pcState().clone();
        pc_ptr->as<PCState>().set((tc->getReg(srcRegIdx(0)) + imm) & ~0x1);
        return std::unique_ptr<PCStateBase>{pc_ptr};
    }

[1]

gem5/src/cpu/o3/decode.cc

Lines 717 to 723 in ffd0680

	std::unique_ptr<PCStateBase> target = inst->branchTarget();
	if (*target != inst->readPredTarg()) {
	++stats.branchMispred;
	// Might want to set some sort of boolean and just do
	// a check at the end
	squash(inst, inst->threadNumber);

@powerjg, @aarmejach: please correct if I'm wrong, my memory is rather vague on O3CPU branch prediction.

[hnpl]

@Ye-Jinhong can you make a PR that fixes this?