On 9/28/23 15:43, Vineet Gupta wrote:
RISC-V suffers from extraneous sign extensions, despite/given the ABI
guarantee that 32-bit quantities are sign-extended into 64-bit registers,
meaning incoming SI function args need not be explicitly sign extended
(so do SI return values as most ALU insns implicitly sign-extend too.)
Existing REE doesn't seem to handle this well and there are various ideas
floating around to smarten REE about it.
RISC-V also seems to correctly implement middle-end hook PROMOTE_MODE
etc.
Another approach would be to prevent EXPAND from generating the
sign_extend in the first place which this patch tries to do.
The hunk being removed was introduced way back in 1994 as
5069803972 ("expand_expr, case CONVERT_EXPR .. clear the promotion flag")
This survived full testsuite run for RISC-V rv64gc with surprisingly no
fallouts: test results before/after are exactly same.
| | # of unexpected case / # of unique unexpected
case
| | gcc | g++ | gfortran |
| rv64imafdc_zba_zbb_zbs_zicond/| 264 / 87 | 5 / 2 | 72 / 12 |
| lp64d/medlow
Granted for something so old to have survived, there must be a valid
reason. Unfortunately the original change didn't have additional
commentary or a test case. That is not to say it can't/won't possibly
break things on other arches/ABIs, hence the RFC for someone to scream
that this is just bonkers, don't do this :-)
I've explicitly CC'ed Jakub and Roger who have last touched subreg
promoted notes in expr.cc for insight and/or screaming ;-)
Thanks to Robin for narrowing this down in an amazing debugging session
@ GNU Cauldron.
So I think Kenner's code is trying to prevent having a value in a SUBREG
that is inconsistent with the SUBREG_PROMOTED* flag bits. But I think
it's been unnecessary since Matz's rewrite in 2009.
The key change in Matz's work is that when the target is a promoted
subreg expression we pass NULL down to expand_expr_real_2 which forces
that routine to expand operand 0 (the incoming PARM_DECL object) into a
temporary object (in this case another promoted subreg expression).
It's that temporary object that Kenner's change clears the promoted
subreg state on.
After expand_expr_real_returns, we call convert_move to move the data
from that temporary object into the actual destination. That routine
(and its children) seem to be doing the right things WRT promoted subregs.
Prior to Matz's change I'm pretty sure we would do expansion directly
into the destination (we'd generate appropriate tree nodes, then
ultimately pass things off to store_expr which would pass down the final
target to expand_expr). Meaning that if the signedness differed then we
potentially needed to reset the subreg promotion state on the
destination object to ensure we were conservatively correct, hence
Kenner's change.
I'm largely convinced we can drop this code. I'm going to give it a few
days to run some of the emulated native targets (they're long running
jobs, so they only fire once a week). But my plan is to move forward
with the patch relatively soon.
jeff
