arc: fix for extvsi split patterns - #270
Conversation
Description change to the pattern define_insn_and_split "*extvsi_n_0" If using && reload_completed the compiler is forced to wait until after the RA/Reload pass is entirely finished. Physical registers are already permanently locked down. If using && 1 the split is allowed to occur early such as during the combine pass when registers are still flexible pseudos. Splitting earlier (using && 1) is generally better than waiting until after reload—provided as it provides significant optimization advantages. Description change to pattern define_insn_and_split "*extvsi_1_0" keeping "0" with && 1 is not in accordance with the GCC instruction pattern rules: 1. According to RTL instruction definition by writing "0" we are telling the dataflow analysis that operand 0 and operand 1 must share the exact same physical register. By writing && 1 we are telling the compiler to split this instruction into pieces during the very early optimization passes (like combine) which is before physical registers are even handed out the "0" constraint. 2. When you split an instruction early using && 1 the purpose is to allow subsequent optimization passes (like Scheduling for register pressure, cse, dce etc) to optimize the broken instructions. If you combine an early split with a restrictive "0" the df-scan gets confused about the exact life ranges of the pseudo registers. Because it thinks the destination and source are linked it may assume a register becomes dead earlier than it actually does. This prevents subsequent passes from safely reordering instructions or eliminating redundant operations resulting in less optimized code.
| (define_insn_and_split "*extvsi_1_0" | ||
| [(set (match_operand:SI 0 "register_operand" "=r") | ||
| (sign_extract:SI (match_operand:SI 1 "register_operand" "0") | ||
| (sign_extract:SI (match_operand:SI 1 "register_operand" "r") |
There was a problem hiding this comment.
I was under the impression that this was a functional issue.
But the resulting RTL pattern is correct?
This is just for performance?
There was a problem hiding this comment.
The first define_insn_and_split has potential to generate incorrect code.
For the second one there was a change from && 1 to &&reload_completed and at the same time there was a change from (sign_extract:SI (match_operand:SI 1 "register_operand" "0")
to (sign_extract:SI (match_operand:SI 1 "register_operand" "r").
My point is that there is no need to have the both as the second change (i.e, (sign_extract:SI (match_operand:SI 1 "register_operand" "0") to (sign_extract:SI (match_operand:SI 1 "register_operand" "r")) is enough allowing the split to take place even before ra.
|
I'm inclined not to go through the effort of arguing to get this merged upstream unless it solves a real functional issue.
df-scan doesn't read the constraints, just checks which registers are filled in the operands in RTL, which are not enforced to be the same before RA. They are, in fact, different registers in the test you made. After RA they will be the same, which might also happen after your change by chance, don't see a problem there. By grepping GCC i found that constraints are only read here: Also the test doesn't fail before the changes in arc.md, so I don't see the point of that either. So again, if this is not a functional issue, let's not bother with this. Don't think this is worth the effort, and let's drop it. |
|
I think we agreed to keep it simple and push it upstream as an non-functional change. |
The test hard-codes two "fcmla ..., #0", which was the number trunk
happened to emit when it was added. BB SLP now also vectorises the two
plain-multiply subroutines: r17-141-g76b8869f08a6 ("tree-optimization/
124222 - rewrite BB SLP costing scalar coverage") taught
vect_bb_slp_scalar_cost to attribute the statements an SLP pattern node
covers, so the subgraph for c_add_ab and c_sub_ab is no longer costed as
unprofitable. They form .COMPLEX_MUL, and cmul<mode>3 expands each to
"fcmla #0" plus "fcmla #90", taking the #0 total from two to four.
For c_add_ab the loop body goes from a scalar pair of complex multiplies
ldr d26, [x5, x0]
ldr d27, [x3, x0]
ldr d29, [x2, x0]
fmul d24, d30, d26
ldr d28, [x1, x0]
fnmsub d24, d31, d27, d24
fmul d25, d30, d27
fmadd d25, d31, d26, d25
fadd d24, d29, d24
fadd d25, d25, d28
str d24, [x2, x0]
str d25, [x1, x0]
add x0, x0, 16
cmp x4, x0
bne .L3
to the vectorised form
ldr q29, [x2, x0]
movi v27.4s, 0
ldr q28, [x3, x0]
fcmla v27.2d, v28.2d, v31.2d, #0
fcmla v27.2d, v28.2d, v31.2d, #90
fadd v27.2d, v27.2d, v29.2d
str q27, [x2, x0]
add x0, x0, 16
cmp x0, x1
bne .L3
i.e. ten instructions instead of fifteen, and the whole rotation mix
across the file changes from
#0=2 #90=0 #180=0 #270=2
to
#0=4 #90=2 #180=0 #270=2
The loop vectoriser is unchanged: its dumps are identical before and
after, the two conjugate subroutines still form .COMPLEX_MUL_CONJ, and
the #270 directive that actually tests PR122408 still passes. The
runtime companion pr122408_2.f90 also still runs clean.
Update the counts, and add a #90 count and a #180 scan-assembler-not.
PR122408 was about picking the wrong rotation pair after operand
swapping, so pinning the full rotation mix guards the regression more
directly than the #0 count did.
gcc/testsuite/ChangeLog:
* gfortran.target/aarch64/pr122408_1.f90: Update the fcmla
rotation counts.
Signed-off-by: Kyrylo Tkachov <ktkachov@nvidia.com>
2 participants
Description change to the pattern define_insn_and_split "*extvsi_n_0"
If using && reload_completed the compiler is forced to wait until after the RA/Reload pass is entirely finished. Physical registers are already permanently locked down. If using && 1 the split is allowed to occur early such as during the combine pass when registers are still flexible pseudos. Splitting earlier (using && 1) is generally better than waiting until after reload—provided as it provides significant optimization advantages.
Description change to pattern define_insn_and_split "*extvsi_1_0" keeping "0" with && 1 is not in accordance with the GCC instruction pattern rules:
According to RTL instruction definition by writing "0" we are telling the dataflow analysis that operand 0 and operand 1 must share the exact same physical register. By writing && 1 we are telling the compiler to split this instruction into pieces during the very early optimization passes (like combine) which is before physical registers are even handed out the "0" constraint.
When you split an instruction early using && 1 the purpose is to allow subsequent optimization passes (like Scheduling for register pressure, cse, dce etc) to optimize the broken instructions. If you combine an early split with a restrictive "0" the df-scan gets confused about the exact life ranges of the pseudo registers. Because it thinks the destination and source are linked it may assume a register becomes dead earlier than it actually does. This prevents subsequent passes from safely reordering instructions or eliminating redundant operations resulting in less optimized code.