Hi everyone,

I recently made a merge request that initally just fixed the incorrect memory alignment for __m128 and similar types, but doing so revealed a whole plethora of other bugs.  First, when I fixed it, __m128 etc were no longer recognised as a valid SIMD or aggregate type due to the wrong alignment field being checked at one point, and some tests with vectorcall revealed some bad code being generated in places.

This may have to be a long work in progress.  I've also found another bug:

program m128test;
function Test3(V1, V2: __m128d): __m128d; vectorcall;
begin
   Test3[0] := V1[0] + V2[0];
   Test3[1] := V1[1] + V2[1];
end;
begin
end.

This will raise Internal error 200410108 under -O2 when compiled under x86_64-win64.  It only occurs with __m128d, not __m128 or __m128i (although __m128i seems to have its own problems).  My merge request fixes the internal error, but produces bad code instead.  When using __m128 or __m128i instead, the following assembly language is produced under -O2:

.section .text.n_p$m128test_$$_test1$__m128$__m128$$__m128,"ax"     .balign 16,0x90
.globl    P$M128TEST_$$_TEST1$__M128$__M128$$__M128 P$M128TEST_$$_TEST1$__M128$__M128$$__M128:
.seh_proc P$M128TEST_$$_TEST1$__M128$__M128$$__M128     leaq    -40(%rsp),%rsp
.seh_stackalloc 40
.seh_endprologue
    movq    %rcx,%rax
    movq    %xmm1,(%rsp)
    movq    %xmm2,8(%rsp)
    movq    %xmm3,16(%rsp)
    movq    %xmm4,24(%rsp)
    movss    (%rsp),%xmm0
    addss    16(%rsp),%xmm0
    movss    %xmm0,(%rax)
    movss    4(%rsp),%xmm0
    addss    20(%rsp),%xmm0
    movss    %xmm0,4(%rax)
    leaq    40(%rsp),%rsp
    ret
.seh_endproc

The fact that the same code is produced under __m128i, which is meant to use integers, is worrying, but that aside, this code is clearly wrong (ignoring the fact that the parameters are being passed on the stack instead of through registers, and %rcx seems to refer to a hidden parameter that's a pointer to the result). -sr reveals that V1 is at (%rsp) and V2 is at 16(%rsp), but the first thing that happens is that their contents are overwritten with undefined values (the movq instructions).  If the operands were reversed, this would seem more logical.

Gareth aka. Kit

P.S. I started making this fix to aid with vectorisation development.

Another problem... I've tried to declare an ADDPD intrinsic as follows:

function x86_addpd(r0, r1: __m128d): __m128d; [INTERNPROC:
fpc_in_x86_addpd];

I thought using __m128d instead of __m128 was fairly logical since ADDPD works with Doubles, not Singles, but this can cause problems.  For example, with the following setup:

     function Special(z1, z2 : __m128d): __m128d; vectorcall;
       begin
         Special := x86_addpd(z1, z2);
       end;

I get the following: Error: Incompatible types: got "__m128" expected "__m128d" (the file location is between the closing bracket and the semicolon of "Special := x86_addpd(z1, z2);" - it seems that it's hard-coded to expect __m128 (error occurs even if vectorcall is not specified).

I know there's still some work to do and things to work out with the vector types and intrinsics.  I will admit that personally I don't like Microsoft Visual C++'s approach of forcing the use of the MM types because it feels inflexible and unfriendly, especially as you might, for example, want to declare and use something like the following?

type TVector4 = packed record
   X, Y, Z, W: Single;
end align 16;

Gareth aka. Kit

Pascal simply is a strongly typed language. Vector intrinsics are no reason to weaken this. Thus you need to declare operator overloads that hide the nitty, gritty details of assigning a TVector4 to a __m128, e.g.:

type
   TVector4 = packed record
     X, Y, Z, W: Single;
     class operator := (const aArg: TVector4): __m128;
     class operator := (const aArg: __m128): TVector4;
   end;

Regards,
Sven

Pascal simply is a strongly typed language. Vector intrinsics are no reason to weaken this. Thus you need to declare operator overloads that hide the nitty, gritty details of assigning a TVector4 to a __m128, e.g.:

That's true - I'm still learning when an implicit typecast is valid and when it isn't.  In the above example, __m128(some_TVector4_type), is usually valid because the alignments and memory are the same.  Internally though, the __m128 types are broken in a few places.

Gareth aka. Kit

Note that the ado_IsVector flag until now was only added to be able to define arrays as vector types for the purpose of the LLVM backend. There is absolutely no support for vector types based on this flag anywhere in the code generator.

Jonas

I used it because it was easy to hot-swap the constructor in the definition of __m128 and the like, and it was a quick and convenient way to ensure the alignment was correct.

Gareth aka. Kit

I mainly meant that some of the bugs you are encountering may be because of that change itself, because it has not been tested/used at all with the regular code generator before.

Jonas

That's fair.  Looks like I'm going to have to explore and experiment a lot more to see if I can 'unbreak' some things.  What was meant to be a simple alignment fix has kind of ballooned into much more.

Gareth aka. Kit

It looks like support for writing to arrays that are wholly stored in registers is a little limited and buggy - while it writes to temporary memory when modifying an individual element, the compiler sometimes doesn't write back the final result into the original register.  I'm seeing if I can fix it.

Gareth aka. Kit

Also I just raised an internal error on the trunk:

program m128test;
type
   UnalignedArray = array[0..1] of Double;
function Test3(V1, V2: UnalignedArray): UnalignedArray; vectorcall;
begin
   Test3[1] := V1[1] + V2[1];
end;
begin
end.

Compiler fails with "m128.pp(7,1) Fatal: Internal error 200410108" under x86_64-win64 (location (7,1) is the "begin" keyword).  I'll be making a bug report of this as a reminder that I need to fix it.  In this situation, the error does not occur if 'vectorcall' is omitted, and the correct behaviour is that the result and parameters aren't vectorised because they aren't aligned to a 16-byte boundary.

(On a side note, I'd ask if it's permissible to add support for "align 16" and the like for regular types and not just records. For example, "type UnalignedArray = array[0..1] of Double align 16;", which is essentially __m128d).

Gareth aka. Kit

J. Gareth Moreton wrote

It looks like support for writing to arrays that are wholly stored in registers is a little limited and buggy

Modifying individual elements of arrays stored in registers has never been enabled nor supported in the compiler. That is why tvecnode.pass_typecheck calls make_not_regable(left,[ra_addr_regable]);

Enabling that will probably require quite a few changes in many different places and possibly for several architectures all over the compiler.

Jonas

That might explain a few things.  The problem is that under vectorcall and the System V ABI (the default x86_64 calling convention for Linux), vector types are supposed to be fully supported, like an aligned array of 4 Singles should be passed in a single XMM register.

I had a feeling that the compiler didn't support it, and you directing me to pass_typecheck for tvecnode is very helpful - thanks.

I'll have to think about this one a bit since, currently, this is a feature specific to x86_64.  I'll have to double-check if there's anything special for AArch64 since that has vector registers too.

https://gitlab.com/freepascal.org/fpc/source/-/issues/39657

Gareth aka. Kit

J. Gareth Moreton wrote

That might explain a few things.  The problem is that under vectorcall and the System V ABI (the default x86_64 calling convention for Linux), vector types are supposed to be fully supported, like an aligned array of 4 Singles should be passed in a single XMM register.

That's no problem in itself. Normally, make_not_regable will ensure that such values will be stored in memory on procedure entry and kept there. Various architectures require that records are also passed in registers (even if they're larger than 1 register), which also work fine even though the compiler only supports record regvars occupying at most one register (or perhaps two, I don't remember).

Jonas

So I've done some testing... make_not_regable is NOT being called at all.  When the array is already on the stack, this is no problem, but if it's in an MM register, then problems start occurring.  If I force the call to make_not_regable, then the bad code disappears.  I'm still learning how it works in this part of the code, especially where the node location changes.

program m128test;
function Test3(V1, V2: __m128d): __m128d; vectorcall;
begin
   Test3[1] := V1[1] + V2[1];
end;
begin
end.

Output:

    # Register rsp,rbp allocated
    pushq    %rbp
.seh_pushreg %rbp
    movq    %rsp,%rbp
    leaq    -64(%rsp),%rsp
.seh_stackalloc 64
# Temp -64,16 allocated
.seh_endprologue
# Temp -16,16 allocated
# Temp -32,16 allocated
# Temp -48,16 allocated
    # Register xmm0,xmm1 allocated
    movdqa    %xmm0,-16(%rbp)
    # Register xmm0 released
    movdqa    %xmm1,-32(%rbp)
    # Register xmm1 released
    # Register mreg32 allocated
    movsd    -8(%rbp),%mreg32md
    addsd    -24(%rbp),%mreg32md
    # Register mreg32 released
    movsd    %mreg32md,-40(%rbp)
# Temp -16,16 released
# Temp -32,16 released
    # Register xmm0 allocated
    movdqa    -48(%rbp),%xmm0
# Temp -48,16 released
# Temp -64,16 released
    leaq    (%rbp),%rsp
    popq    %rbp
    # Register rbp,rsp released
    ret
.seh_endproc
    # Register xmm0 released

No uninitialised registers are being written (although -48(%rbp) is uninitialised because I don't write to Test3[0]).  Of course I don't want to force the call to make

Since this feature is still a work in progress and bugs are inevitable, my merge request over here now only does what it was originally designed to do... make __m128 and the like aligned, although admittedly code that ensures they are treated as vector types is tied into the same commit:

https://gitlab.com/freepascal.org/fpc/source/-/merge_requests/193

Currently I'm focusing on fixing the Internal Error and some related problems I found:
https://gitlab.com/freepascal.org/fpc/source/-/issues/39657

Gareth aka. Kit