-
Notifications
You must be signed in to change notification settings - Fork 234
Added rm field in assembly for fcvt instructions and fixed default to dyn. #271
base: riscv-binutils-2.38
Are you sure you want to change the base?
Conversation
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 |
I support the general idea. On disassembler, it can be great! Problems are:
So, I'll make another proposal based on your idea. |
Wait... making Quoting RISC-V ISA Manual (13.2 Floating-Point Control and Status Register):
Actually, |
@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 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. |
I dont quite understand what you mean here.
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.
|
7f29f2e
to
46f218d
Compare
46f218d
to
beb1a46
Compare
@aswaterman Just make sure you mean all fcvt instructions instead of all floating point instructions, right? |
@kito-cheng I was specifically referring to instructions that are unaffected by rounding mode: e.g., For any instruction that does round (e.g. |
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).
|
Fixes issues outlined here.