Incorrect instruction encoding for JAL

[charlesdaniels]

Minimal test case:

jal x1 l1
l1:
nop

This assembles to:

 Address    Code        Basic                     Source

0x00000000  0x004000ef  jal x1,0x00000002     1    jal x1 l1
0x00000004  0x00000013  addi x0,x0,0x00000000 3    nop

(This is compact .text at 0, but it doesn't matter since JAL is relative)

However, according to the spec on page 16, the low order bit of the immediate value is placed at index 21. This would imply the instruction should be encoded as 0x002000ef -- noting that 0x002000ef & (1 << 21) >> 21 = 1. Also note that JAL technically has a 21-bit immediate, but the lowest order bit is omitted as described on page 15, thus a a relative jump to PC+2 should have a single 1 in the lowest order bit of the immediate.

[TheThirdOne]

I am confident that the encoding is correct. Using GNU as followed by objdump results in the following which confirms 004000ef as the correct encoding.

0000000000000000 <l1-0x4>:
   0:	004000ef          	jal	ra,4 <l1>

0000000000000004 <l1>:
   4:	00000013          	nop

I think that the "Basic" representation is highly misleading. I would suspect it is the reason why you made this issue. In terms of the specification, the immediate for this jal should be immediate[20:0] = 4. However the value shown in the "Basic" representation is immediate[20:1] which is not intuitive.

Currently all code interacting with Jump (and Branch) immediates uses a divided by 2 representation. It is probably best to replace that with a direct immediate representation. Such a change would require changing the Jump immediate decode and encode as well as all the areas where instructions use them.

rars/src/rars/ProgramStatement.java

Lines 409 to 426 in 41fff8f

	private int toJumpImmediate(int address) {
	// trying to produce immediate[20:1] where immediate = address[20|10:1|11|19:12]
	return (address & (1 << 19)) | // keep the top bit in the same place
	((address & 0x3FF) << 9) | // move address[10:1] to the right place
	((address & (1 << 10)) >> 2) | // move address[11] to the right place
	((address & 0x7F800) >> 11); // move address[19:12] to the right place
	}
	private int fromJumpImmediate(int immediate) {
	// trying to produce address[20:1] where immediate = address[20|10:1|11|19:12]
	int tmp = ((immediate) & (1 << 19)) | // keep the top bit in the same place
	((immediate & 0x7FE00) >> 9) | // move address[10:1] to the right place
	((immediate & (1 << 8)) << 2) |// move address[11] to the right place
	((immediate & 0xFF) << 11); // move address[19:12] to the right place
	return (tmp << 12) >> 12; // sign-extend
	}

rars/src/rars/riscv/instructions/JAL.java

Line 46 in 41fff8f

InstructionSet.processJump(RegisterFile.getProgramCounter() - Instruction.INSTRUCTION_LENGTH + (operands[1] << 1));

Thank you for making this issue. If you want to try to fix it, it should be pretty simple aside from confirming all relevant code has been changed. Alternatively, I can fix it in a few days when I am free.

[charlesdaniels]

Indeed, I was confused by the "basic representation". Thank you for your speedy and detailed explanation!

[TheThirdOne]

This should be fixed now. Let me know if it seems correct to you in the continuous build.

[charlesdaniels]

Yes, now what the "Basic" column shows is in line with what I would expect.