Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small suggestion about PC #1

Closed
chegewara opened this issue Feb 6, 2021 · 10 comments
Closed

Small suggestion about PC #1

chegewara opened this issue Feb 6, 2021 · 10 comments
Assignees

Comments

@chegewara
Copy link

Hi, i really like how you are explaining and doing all this stuff, but i think you have 1 wrong assumption.
PC should be increased before instruction is parsed/exacuted:
https://github.com/wokwi/rp2040js/blob/master/src/rp2040.ts#L151

This will let you fix the branch tests:

    it('should execute a `b.n	.-18` instruction', () => {
      const rp2040 = new RP2040('');
      rp2040.PC = 9 * 2;
      rp2040.flash16[9] = 0xe7f6; // b.n	.-18
      rp2040.executeInstruction();
      expect(rp2040.PC).toEqual(0);
    });

    it('should execute a `bne.n .-6` instruction', () => {
      const rp2040 = new RP2040('');
      rp2040.PC = 9 * 2;
      rp2040.Z = false;
      rp2040.flash16[9] = 0xd1fc; // bne.n .-6
      rp2040.executeInstruction();
      expect(rp2040.PC).toEqual(12);
    });

For confirmation here is pseudocode for BL:

if ConditionPassed() then
 EncodingSpecificOperations();
 next_instr_addr = PC;
 LR = next_instr_addr<31:1> : ‘1’;
 BranchWritePC(PC + imm32);

As you can see next instruction address store is PC, which cant be PC to currently running instruction or this will stuck in recurent loop.

Another small suggestion about finding the right instruction in datasheet, you can use hex calculator.
This snippet is good too to sign extend any value, like imm5, imm8, imm11:

function signExtend(value: number, bits: number) {
  return value & (1<<bits-1) ? 0x80000000 + (value & ((1<<bits-1)-1)) : value;
}

Thanks

@urish urish self-assigned this Feb 6, 2021
@urish
Copy link
Contributor

urish commented Feb 6, 2021

Thank you!

Regarding PC, the I found that the ARM® v6-M manual explains it in section A4.2.1 (Use of labels in UAL instruction syntax):

  • "The PC value of an instruction is its address plus 4 for a Thumb instruction"
  • "The Align(PC,4) value of an instruction is its PC value ANDed with 0xFFFFFFFC to force it to be word-aligned."

I will incorporate that into the code in our next session on Tuesday.

@chegewara
Copy link
Author

chegewara commented Feb 6, 2021

Just as reminder (A.5.1.2):

Read the word-aligned PC value, that is, the address of the current instruction + 4, with bits [1:0] forced to
zero. This enables instructions such as ADR and LDR (literal) instructions to use PC-relative data addressing. 

Which works when you increment PC before execute instruction.

@urish
Copy link
Contributor

urish commented Feb 6, 2021

I'm wondering why it uses "current instruction + 4" as the baseline, as instructions are only 2 bytes large (other than BL if I'm not wrong). It seems like even if we pre-increment, we'll have to add 2 to the value of PC when we are using it in offset-relative calculations

@chegewara
Copy link
Author

It is exactly the same you posted earlier, just described with different words:

Thank you!

Regarding PC, the I found that the ARM® v6-M manual explains it in section A4.2.1 (Use of labels in UAL instruction syntax):

  • "The PC value of an instruction is its address plus 4 for a Thumb instruction"
  • "The Align(PC,4) value of an instruction is its PC value ANDed with 0xFFFFFFFC to force it to be word-aligned."

I will incorporate that into the code in our next session on Tuesday.

It is aligned PC only for few instructions, like ADR or LDR, but again, i played a bit with your code today and it works only when PC is incemented before instruction is excuted. Incementing PC at top is fixing also the problem with b.n or bne.n in tests. In code it works fine, because you actually are fooling code with
https://github.com/wokwi/rp2040js/blob/master/src/rp2040.ts#L168

of course you dont need this line, which is moved into top:
https://github.com/wokwi/rp2040js/blob/master/src/rp2040.ts#L284

@sutaburosu
Copy link

sutaburosu commented Feb 6, 2021

I'm wondering why it uses "current instruction + 4" as the baseline, as instructions are only 2 bytes large

Thumb instructions are only 2-bytes long, but after a BX opcode, on ARMs which support it, the ARM instruction set will be used instead; they are all 4-bytes long. This PC+4 offset goes back to the earliest revisions of ARM, before the Thumb instructions were added. I guess the choice was made to use PC+4 even in Thumb mode to reduce confusion.

@urish
Copy link
Contributor

urish commented Feb 6, 2021

Interestingly, in non-Thumb mode it's actually PC+8: "The PC value of an instruction is its address plus 4 for a Thumb instruction, or plus 8 for an ARM instruction."

@sutaburosu
Copy link

Ah! On this ARM variant, PC actually points 2 instructions after the current one, because they deepened the pipeline from 3- to 5-stages. It surprises me that they chose to expose this extra 1 instruction offset. DEC/Intel's StrongARM also had a similar 5-stage pipeline, but still only a +1 instruction offset in PC.

@urish
Copy link
Contributor

urish commented Feb 6, 2021

Cool, thanks for the link!

@urish
Copy link
Contributor

urish commented Feb 16, 2021

Thanks again for the suggestion, @chegewara! I ended up implementing it at the beginning of last week's stream, and it actually helped fixed the bug we were looking at. So great timing too :-)

@urish urish closed this as completed Feb 16, 2021
@chegewara
Copy link
Author

No problem, and i saw last video. I will watch whole series, because its very nice and even if i dont plan to use it, most likely, it is very good to learn new things and new way of thinking. Good luck.

urish added a commit that referenced this issue Feb 23, 2021
new instructions: ADR, BICS, BLX, BX (thanks Tony #1), LDRH, POP, STMIA.

next time, we need to implement:
- 4198      	sbcs	r0, r3
- 4643      	mov	r3, r8
- 4005        ands	r5, r0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants