Hi,
For this code:
struct y {
float x[4];
};
float
bar3 (struct y x)
{
return x.x[3];
}
GCC generates:
bar3:
fmov x1, d2
mov x0, 0
bfi x0, x1, 0, 32
fmov x1, d3
bfi x0, x1, 32, 32
sbfx x0, x0, 32, 32
fmov s0, w0
ret
If you can wrap your head around that, you'll spot that it could be
simplified to:
bar3:
fmov s0, s3
ret
Looking at it, I think the issue is the mode that we assign to the
PARALLEL we build for an HFA in registers. When we get in to
aarch64_layout_arg with a composite, MODE is set to the smallest integer
mode that would contain the size of the composite type. That is to say, in
the example above, MODE will be TImode.
Looking at the expansion path through assign_parms, we're going to go:
assign_parms
assign_parm_setup_reg
assign_parm_remove_parallels
emit_group_store
assign_parm_remove_parallels is going to try to create a REG in MODE,
then construct that REG using the values in the HFA PARALLEL we created. So,
for the example above, we're going to try to create a packed TImode value
built up from each of the four "S" registers we've assigned for the
arguments. Using one of the struct elements is then a 32-bit extract from
the TImode value (then a move back to FP/SIMD registers). This explains
the code-gen in the example. Note that an extract from the TImode value
makes the whole TImode value live, so we can't optimize away the
construction in registers.
If instead we make the PARALLEL that we create in aarch64_layout_arg BLKmode
then our expansion path is through:
assign_parms
assign_parm_setup_block
Which handles creating a stack slot of the right size for our HFA, and
copying it to there. We could then trust the usual optimisers to deal with
the object construction and eliminate it where possible.
However, we can't just return a BLKmode Parallel, as the mid-end was explictly
asking us to return in MODE, and will eventually ICE given the inconsistency.
One other way we can force these structures to be given BLKmode is through
TARGET_MEMBER_TYPE_FORCES_BLK. Which is what we do in this patch.
We're going to tell the mid-end that any structure of more than one element
which contains either floating-point or vector data should be set out in
BLKmode rather than a large-enough integer mode. In doing so, we implicitly
fix the issue with HFA layout above. But at what cost!
A long running deficiency in GCC's code-gen (doesn't clean up stack
allocations after stack uses have been eliminated) prevents us from
getting what we really wanted, but:
bar3:
sub sp, sp, #16
fmov s0, s3
add sp, sp, 16
ret
is pretty close, and a huge improvement over where we are today.
Note that we can still get some pretty bad code-generation out of the
compiler when passing and returning structs. I particularly like this
one:
struct y {
float x[4];
};
struct y
bar (struct y x)
{
return x;
}
bar:
sub sp, sp, #48
stp s0, s1, [sp, 16]
stp s2, s3, [sp, 24]
ldp x0, x1, [sp, 16]
stp x0, x1, [sp, 32]
ldp s0, s1, [sp, 32]
ldp s2, s3, [sp, 40]
add sp, sp, 48
ret
But that looks to be a seperate issue, and is not substantially worse tha
current trunk:
bar:
fmov x2, d0
mov x1, 0
mov x0, 0
bfi x1, x2, 0, 32
fmov x2, d2
bfi x0, x2, 0, 32
fmov x2, d1
bfi x1, x2, 32, 32
fmov x2, d3
bfi x0, x2, 32, 32
ubfx x2, x1, 0, 32
ubfx x1, x1, 32, 32
fmov s0, w2
ubfx x3, x0, 0, 32
fmov s1, w1
ubfx x0, x0, 32, 32
fmov s2, w3
fmov s3, w0
ret
I've benchamrked this with Spec2000 and found no performance differences.
And bootstrapped on aarch64-none-linux-gnu with no issues.
Does this look like a sensible approach and if so, is it OK for trunk?
Thanks,
James
---
gcc/
2017-06-20 James Greenhalgh <james.greenha...@arm.com>
* config/aarch64/aarch64.c (aarch64_layout_arg): Construct HFA
PARALLELs in BLKmode.
gcc/testsuite/
2017-06-20 James Greenhalgh <james.greenha...@arm.com>
* gcc.target/aarch64/hfa_1.c: New.
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 04417dc..a147068 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -14925,6 +14925,32 @@ aarch64_sched_can_speculate_insn (rtx_insn *insn)
}
}
+/* We're an composite type, so MODE is the smallest integer mode
+ that can fit the total size of our aggregate. However,
+ we're going to build a parallel that contains each of our
+ registers, and GCC is going to emit code to move them in
+ to a packed value in MODE. As an example, for an HFA of
+ two floats, this means creating a packed DImode value from
+ which we can then extract the SFmode elements. This adds
+ considerable inefficiency. We can avoid that issue by
+ creating the parallel with BLKmode, which forces the
+ the structure to be constructed on the stack.
+ Optimisation passes may then elide the stack stores if
+ possible. */
+
+static bool
+aarch64_member_type_forces_blk (const_tree field, machine_mode mode)
+{
+ machine_mode field_mode = TYPE_MODE (TREE_TYPE (field));
+ /* If we are a multi-element structure, and any of those elements are
+ floating-point or vector types then move the whole thing to BLKmode
+ to prevent the silly compiler from deciding to build great big stonking
+ integers full of HFA data. Though presumably this is going to blow
+ copies of those structures, which is annoying. */
+ return (mode == VOIDmode
+ && (SCALAR_FLOAT_MODE_P (field_mode)));
+}
+
/* Target-specific selftests. */
#if CHECKING_P
@@ -15115,6 +15141,9 @@ aarch64_libgcc_floating_mode_supported_p
#undef TARGET_MANGLE_TYPE
#define TARGET_MANGLE_TYPE aarch64_mangle_type
+#undef TARGET_MEMBER_TYPE_FORCES_BLK
+#define TARGET_MEMBER_TYPE_FORCES_BLK aarch64_member_type_forces_blk
+
#undef TARGET_MEMORY_MOVE_COST
#define TARGET_MEMORY_MOVE_COST aarch64_memory_move_cost
diff --git a/gcc/testsuite/gcc.target/aarch64/hfa_1.c b/gcc/testsuite/gcc.target/aarch64/hfa_1.c
new file mode 100644
index 0000000..55c22cf
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/hfa_1.c
@@ -0,0 +1,34 @@
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+struct x {
+ float x[4];
+};
+
+float
+foo0 (struct x x)
+{
+ return x.x[0];
+}
+
+float
+foo1 (struct x x)
+{
+ return x.x[1];
+}
+
+float
+foo2 (struct x x)
+{
+ return x.x[2];
+}
+
+
+float
+foo3 (struct x x)
+{
+ return x.x[3];
+}
+
+/* We don't want to see any moves to the integer register bank. */
+/* { dg-final { scan-assembler-not "fmov\\t\[wx\]\[0-9\]+"} } */
