search

riscvarchive / riscv-binutils-gdb

RISC-V backports for binutils-gdb. Development is done upstream at the FSF.
GNU General Public License v2.0
148 stars 233 forks source link

Added rm field in assembly for fcvt instructions and fixed default to dyn. #271

Open pawks opened 2 years ago

pawks commented 2 years ago

Fixes issues outlined here.

cmuellner commented 2 years ago

Thanks for your contribution!

Binutils development is done upstream (https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git). Patches can be sent to the binutils mailing list (https://sourceware.org/mailman/listinfo/binutils).

Also, please make sure to adjust test cases that are affected by your changes (have a look into gas/testsuite/gas/riscv).

a4lg commented 2 years ago

I support the general idea. On disassembler, it can be great!

Problems are:

So, I'll make another proposal based on your idea.

a4lg commented 2 years ago

Wait... making DYN default for certain instructions is not good.

Quoting RISC-V ISA Manual (13.2 Floating-Point Control and Status Register):

Some instructions, including widening conversions, have the rm field but are nevertheless mathematically unaffected by the rounding mode; software should set their rm field to RNE (000) but implementations must treat the rm field as usual (in particular, with regard to decoding legal vs. reserved encodings).

Actually, fcvt.q.l[u] instructions are fixed to have RNE by default (instead of DYN) in 1d1595b48b7146952bdd2f7c90607977dd0c89d6.

aswaterman commented 2 years ago

@a4lg speaking as one of the authors of this part of the spec, I agree that defaulting to DYN is not the preference.

Fortunately, this is a very minor issue in practice. Most importantly, there is no functional-correctness concern. There is a hypothetical loss in performance when frm is explicitly written shortly before an instruction implicitly reads frm because the rounding mode is DYN, but rarely should this loss manifest.

I support changing the default to RNE for all instructions that don't round. There would be no functional implications to this change, and it would bring the assembler more in line with the ISA spec's recommendations.

pawks commented 2 years ago

I support the general idea. On disassembler, it can be great!

Problems are:

  • You are based on quite an old revision.

I dont quite understand what you mean here.

  • Prohibiting use of rounding mode operand in the assembler may be an option.

The assembler should be supporting the rounding modes since it is a legal field in the specification. That was the motivation behind this PR. I don't think adding the rm modes breaks anything. Although having a default of RNE seems to be for the best.

So, I'll make another proposal based on your idea.

kito-cheng commented 2 years ago

@aswaterman Just make sure you mean all fcvt instructions instead of all floating point instructions, right?

aswaterman commented 2 years ago

@kito-cheng I was specifically referring to instructions that are unaffected by rounding mode: e.g., fcvt.d.w[u] and fcvt.d.s.

For any instruction that does round (e.g. fcvt.s.w or fadd.s), dyn needs to remain the default--both because it's the most logical default, and because it's necessary for backwards compatibility.

a4lg commented 2 years ago

My initial proposal (I will submit later to binutils ML) is rather conservative but can be easily modified to implement your proposal (by just removing warning/error generating portion).