On 1/12/25 10:44 PM, Zhijin Zeng wrote:
Compared to the patch v2, I added Zfinx check and Zfh check. Please help to
review again.
Thanks,
Zhijin
From 9ddb402cebe868050ebc2f75e4d87238161411b4 Mon Sep 17 00:00:00 2001
From: Zhijin Zeng <zhijin.z...@spacemit.com>
Date: Sat, 11 Jan 2025 12:09:11 +0800
Subject: [PATCH] RISC-V: Fix mode compatibility between floating-point and
integer value
I find there are some unnecessary fmv instructions in glibc math exp,
and reduce exp function to the attached test case. The unnecessary
fmv instructions as follow:
```
fld fa4,16(a4)
fmadd.d fa2,fa2,fa0,fa5
fld fa0,56(a4)
fmv.x.d a5,fa2 *****
fld fa2,48(a4)
fmv.d.x fa1,a5 *****
andi a3,a5,127
addi a3,a3,15
fsub.d fa5,fa1,fa5
slli a3,a3,3
```
So just to be clear, this is one of those cases where we want to work on
the value as both an FP value and an integer value. These cases are
notoriously hard to make "perfect".
The data of fa2 and fa1 are the same, we should directly use fa2
rather than fa1 in following instructions and save one fmv instruction.
Ideally, yes. But note that you may well fix this case and make others
perform poorly. That really just means we have to be careful.
The `fmv.d.x a5,fa2` is correspond to pattern `(subreg:DI (reg/v:DF 143`.
In ira pass, virtual register r143 is assigned to GP_REGS, so its data
need be copied to FP_REGS before `fsub.d fa5,fa1,fa5` by reload pass,
and that's exactly the `fmv.d.x fa1,a5` instruction.
Right. We've a pseudo that is primarily used in FP instructions, but
which is also used in an integer context. This is an artifact of fwprop.
Prior to fwprop we had:
(insn 13 12 14 2 (set (reg:DI 144 [ _16 ])
(subreg:DI (reg/v:DF 143 [ kd ]) 0)) "j.c":44:5 277 {*movdi_64bit}
(nil))
The net is that prior to fwprop things looked pretty sensible. We had
r143 for use in the FP instructions and r144 for use in integer
instructions.
fwprop propagates away the subreg copy resulting in using r143 in both
the scalar and FP contexts. Given the current definition of
modes_tieable_p, that's a sensible decision, though it's causing
problems later.
Changing modes_tieable_p would be the way to prevent that behavior in
fwprop. But I don't think that's really the right change.
Note carefully modes_tieable_p has no notion of register files. It
works strictly on modes. Some ports do use modes as a rough proxy for
register files, but it's far from clear if it's the best way forward for
RISC-V.
So if we're going to change modes_tieable_p like this we really need to
do deeper analysis than looking at a few routines from glibc. It's the
kind of change that would normally be tested on designs with spec.
I don't know if you have access to spec or not. If not, then this
probably needs to wait for someone who has the time to do a deeper dive.
3. JALR_REGS is also a subset of GR_REGS and need to be taken
into acount in riscv_register_move_cost, otherwise it will get
a incorrect cost.
This looks like a pretty straightforward bugfix. Note that we probably
also want to handle SIBCALL_REGS in a manner similar to JALR_REGS since
it's a subset of the GPR regsiter file.
Can you break out that change separately and include SIBCALL_REGS?
Barring any surprises that should be able to go forward immediately.
Jeff
