On 8/7/24 12:27 AM, Christoph Müllner wrote:
We have a huge amount of optimization patterns (insn_and_split) for
XTheadMemIdx and XTheadFMemIdx that attempt to do something, that can be
done more efficient by generic GCC passes, if we have proper support code.
A key function in eliminating the optimization patterns is
th_memidx_classify_address_index(), which needs to identify each possible
memory expression that can be lowered into a XTheadMemIdx/XTheadFMemIdx
instruction. This patch adds all memory expressions that were
previously only recognized by the optimization patterns.
Now, that the address classification is complete, we can finally remove
all optimization patterns with the side-effect or getting rid of the
non-canonical memory expression they produced: (plus (reg) (ashift (reg)
(imm))).
A positive side-effect of this change is, that we address an RV32 ICE,
that was caused by the th_memidx_I_c pattern, which did not properly
handle SUBREGs (more details are in PR116131).
A temporary negative side-effect of this change is, that we cause a
regression of the xtheadfmemidx + xtheadfmv/zfa tests (initially
introduced as part of b79cd204c780 to address an ICE).
As this issue cannot be addressed in the code parts that are
adjusted in this patch, we just accept the regression for now.
PR target/116131
gcc/ChangeLog:
* config/riscv/thead.cc (th_memidx_classify_address_index):
Recognize all possible XTheadMemIdx memory operand structures.
(th_fmemidx_output_index): Do strict classification.
* config/riscv/thead.md (*th_memidx_operand): Remove.
(TARGET_XTHEADMEMIDX): Likewise.
(TARGET_HARD_FLOAT && TARGET_XTHEADFMEMIDX): Likewise.
(!TARGET_64BIT && TARGET_XTHEADMEMIDX): Likewise.
(*th_memidx_I_a): Likewise.
(*th_memidx_I_b): Likewise.
(*th_memidx_I_c): Likewise.
(*th_memidx_US_a): Likewise.
(*th_memidx_US_b): Likewise.
(*th_memidx_US_c): Likewise.
(*th_memidx_UZ_a): Likewise.
(*th_memidx_UZ_b): Likewise.
(*th_memidx_UZ_c): Likewise.
(*th_fmemidx_movsf_hardfloat): Likewise.
(*th_fmemidx_movdf_hardfloat_rv64): Likewise.
(*th_fmemidx_I_a): Likewise.
(*th_fmemidx_I_c): Likewise.
(*th_fmemidx_US_a): Likewise.
(*th_fmemidx_US_c): Likewise.
(*th_fmemidx_UZ_a): Likewise.
(*th_fmemidx_UZ_c): Likewise.
gcc/testsuite/ChangeLog:
* gcc.target/riscv/pr116131.c: New test.
Nice cleanup. I did wander the old PA code a bit most of what found was
actually addressing limitations due to its weird implicit segment
selection based on the base register rather than the full effective
address. Bad memories.
But that was enough to get a few synapses to fire. When I last looked at
the canonicalization issues (circa 2015 IIRC) in this space I adjusted
combine to handle things better. So probably not nearly as much to do
in the target files anymore.
I'll note the thead code is riddled with explicit mode testing which
looks less than ideal, but that's a pre-existing issue and it may in
fact be reasonable, I don't know the thead extensions well enough to be
100% sure either way. So I won't object, but there's a bit of "ewww"
when I look at the thead address classification code.
It looks like the pre-commit tester didn't test patches #2 or #3. So
while I'll ack, please be on the lookout for any failures.
Jeff
