On 26/09/14 02:16, Charles Baylis wrote:
On 19 September 2014 12:21, Tejas Belagod <tejas.bela...@arm.com> wrote:
The reason we avoided using type-punning using unions was that reload would
get confused with potential subreg(mem) that could be introduced because of
memory xfer caused by unions and large int modes. As a result, we would get
incorrect or sub-optimal code. But this seems to have fixed itself. :-)
Because this involves xfers between large int modes and
CANNOT_CHANGE_MODE_CLASS has some impact on it, it would be good to test
what impact your patch has with C_C_M_C removed, so that it will be easier
to fix the fallout once we remove C_C_M_C eventually. To test this you will
need Richard's patch set
https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01440.html.
Same for your other 2 patches in this series(3,4).
I tried those patches, and altered aarch64_cannot_change_mode_class to
return false for all cases.
However, this does not avoid the unnecessary moves.
Taking a really simple test case:
#include <arm_neon.h>
int32x2x2_t xvld2_s32(int32_t *__a)
{
int32x2x2_t ret;
__builtin_aarch64_simd_oi __o;
__o = __builtin_aarch64_ld2v2si ((const __builtin_aarch64_simd_si *) __a);
ret.val[0] = (int32x2_t) __builtin_aarch64_get_dregoiv2si (__o, 0);
ret.val[1] = (int32x2_t) __builtin_aarch64_get_dregoiv2si (__o, 1);
return ret;
}
(disabling scheduling for clarity)
$ aarch64-oe-linux-gcc -O2 -S -o - simd.c -fno-schedule-insns
-fno-schedule-insns2
...
xvld2_s32:
ld2 {v2.2s - v3.2s}, [x0]
orr v0.8b, v2.8b, v2.8b
orr v1.8b, v3.8b, v3.8b
ret
...
The reason is apparent in the rtl dump from ira:
...
Allocno a0r73 of FP_REGS(32) has 31 avail. regs 33-63, node:
33-63 (confl regs = 0-32 64 65)
...
(insn 2 4 3 2 (set (reg/v/f:DI 79 [ __a ])
(reg:DI 0 x0 [ __a ])) simd.c:5 34 {*movdi_aarch64}
(expr_list:REG_DEAD (reg:DI 0 x0 [ __a ])
(nil)))
(note 3 2 6 2 NOTE_INSN_FUNCTION_BEG)
(insn 6 3 20 2 (set (reg/v:OI 73 [ __o ])
(subreg:OI (vec_concat:V8SI (vec_concat:V4SI (unspec:V2SI [
(mem:TI (reg/v/f:DI 79 [ __a ]) [0 S16 A8])
] UNSPEC_LD2)
(vec_duplicate:V2SI (const_int 0 [0])))
(vec_concat:V4SI (unspec:V2SI [
(mem:TI (reg/v/f:DI 79 [ __a ]) [0 S16 A8])
] UNSPEC_LD2)
(vec_duplicate:V2SI (const_int 0 [0])))) 0))
simd.c:8 2149 {aarch64_ld2v2si_dreg}
(expr_list:REG_DEAD (reg/v/f:DI 79 [ __a ])
(nil)))
(insn 20 6 21 2 (set (reg:V2SI 32 v0)
(subreg:V2SI (reg/v:OI 73 [ __o ]) 0)) simd.c:12 778
{*aarch64_simd_movv2si}
(nil))
(insn 21 20 22 2 (set (reg:V2SI 33 v1)
(subreg:V2SI (reg/v:OI 73 [ __o ]) 16)) simd.c:12 778
{*aarch64_simd_movv2si}
(expr_list:REG_DEAD (reg/v:OI 73 [ __o ])
(nil)))
(insn 22 21 23 2 (use (reg:V2SI 32 v0)) simd.c:12 -1
(nil))
(insn 23 22 0 2 (use (reg:V2SI 33 v1)) simd.c:12 -1
(nil))
The register allocator considers r73 to conflict with v0, because they
are simultaneously live after insn 20. Without the 2nd use of v73 (eg
if the write to res.val[1] is replaced with vdup_n_s32(0) ) then the
allocator does do the right thing with the subreg and allocates v73 to
{v0,v1}.
I haven't read all of the old threads relating to Richard's patches
yet, but I don't see why they would affect this issue.
I don't think the register allocator is able to resolve this unless
the conversion between the __builtin_simd type and the int32x4x2_t
type is done as a single operation.
For this piece of code,
#include "arm_neon.h"
int32x2x2_t xvld2_s32(int32_t *__a)
{
union { int32x2x2_t __i;
__builtin_aarch64_simd_oi __o; } __temp;
__temp.__o = __builtin_aarch64_ld2v2si ((const
__builtin_aarch64_simd_si *) __a);
return __temp.__i;
}
int32x2x2_t yvld2_s32(int32_t *__a)
{
int32x2x2_t ret;
__builtin_aarch64_simd_oi __o;
__o = __builtin_aarch64_ld2v2si ((const __builtin_aarch64_simd_si *)
__a);
ret.val[0] = (int32x2_t) __builtin_aarch64_get_dregoiv2si (__o, 0);
ret.val[1] = (int32x2_t) __builtin_aarch64_get_dregoiv2si (__o, 1);
return ret;
}
currently my gcc HEAD generates at -O3:
xvld2_s32:
ld2 {v0.2s - v1.2s}, [x0]
sub sp, sp, #64
st1 {v0.16b - v1.16b}, [sp]
ldr x1, [sp]
ldr x0, [sp, 8]
add sp, sp, 64
ins v0.d[0], x1
ins v1.d[0], x0
ret
....
yvld2_s32:
ld2 {v2.2s - v3.2s}, [x0]
orr v1.8b, v3.8b, v3.8b
orr v0.8b, v2.8b, v2.8b
ret
If we use type-punning, there are unnecessary spills that are generated
which is also incorrect for BE because of of the way we spill (st1
{v0.16b - v1.16b}, [sp]) and restore. The implementation without
type-punning seems to give a more optimal result. Did your patches
improve on the spills for the type-punning solution?
However, type-punning is not possible with the arrays of 64 bit
vectors, as the arrays are not the same size as the corresponding
__builtin_simd types, and any solution for those would probably help
with the q variants too.
That is because we fill a zero-extended D-reg value into a 128-bit reg
and pack them into an large int mode(eg. OI). We don't have large int
modes made up of purely D-regs because we run into ambiguities like 4
D-regs is an OImode and 2 Q-regs is also an OImode.
Maybe the solution is to pass the NEON
intrinsic types directly to the builtins? Is there a reason that it
wasn't done that way before?
How do you mean? Do you mean pass a loaded value int32x2x2_t into a
__builtin? How will that work?
If you mean why we don't pass an int32x2x2_t into a builtin as a
structure, I don't think that would work as it is struct type which
would correspond to a BLK mode, but we need RTL patterns with reg-lists
to work with large int modes for the regalloc to allocate consecutive
regs for the reglists.
Thanks,
Tejas.
