Skip to content
This repository has been archived by the owner on Aug 17, 2022. It is now read-only.

nop and addi x0, x0, 0 should be identical, are not #135

Closed
sorear opened this issue Jan 15, 2018 · 4 comments
Closed

nop and addi x0, x0, 0 should be identical, are not #135

sorear opened this issue Jan 15, 2018 · 4 comments

Comments

@sorear
Copy link

sorear commented Jan 15, 2018

According to the "Assembly Programmer's Handbook" in the ISA manual, nop is a pseudo-instruction and is equivalent to addi x0, x0, 0. However the current assembler treats them differently and is unable to output a compressed encoding for the latter:

$ cat asdf.s
floor:
        nop
        addi x0, x0, 0
$ as -march=rv32ic asdf.s
$ objdump -d a.out

a.out:     file format elf32-littleriscv


Disassembly of section .text:

00000000 <floor>:
   0:   0001                nop
   2:   00000013            nop

If this behavior is deliberate it should be documented somewhere.

Context: http://lkml.iu.edu/hypermail/linux/kernel/1801.1/06322.html

@jim-wilson
Copy link
Collaborator

It looks like an oversight. In the base instruction set, nop is an alias for addi, but in the compressed intsruction set, c.nop and a.addi are different instructions with similar encodings. c.addi with a zero immediate is a hint. c.nop with a non-zero immediate is a hint (in v2.3 draft). So we can't compress addi x0,x0,0 to c.addi, but we could compress it to c.nop. A rule for that is missing in the opcode table.

@sorear
Copy link
Author

sorear commented Jan 15, 2018

My perspective is that there are four C instructions corresponding to base addi, c.addi, c.addi4spn, c.addi16sp, and c.nop, and they should all be handled; the current behavior additionally breaks the conceptual model where pseudoinstructions are expanded before compression.

@jim-wilson
Copy link
Collaborator

c.addi, c.addi4spn, and c.addi16sp are already handled. It is only the c.nop support that is missing.

@jim-wilson
Copy link
Collaborator

I added a patch upstream. This just missed the binutils 2.30 branch. I don't think it is important enough to backport to that release branch, which would require additional testing for little or no gain.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants