On Fri, Nov 6, 2009 at 04:11, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Nov 05, 2009 at 05:21:24PM +0100, Jakub Jelinek wrote:
>> As the following patch proves, lwpintrin.h and the whole lwp support is
>> quite severely broken.
>

I realized the brokenness of the LWP support when I had to work on
correcting the following points.  Sorry.

First I am sending out a preliminary patch that does fix some of the
points below, but that still does not pass the testsuite with your
patch applied.

> As the following patch proves, lwpintrin.h and the whole lwp support is
> quite severely broken.
>
> 1) llwpcb* - the builtins are declared void __builtin_ia32_llwpcb* (void),
> but lwpintrin.h expects them to take void * argument.

Fixed.

> If I understand right, the
> insn in reality has 3 address sizes to support 16-bit/32-bit/64-bit code,
> I fail to see why we'd need 3 different intrinsics, instead of just one and
> one builtin that takes void * address and uses the insn matching Pmode.

Unless I am doing something wrong, I remarked that the HI mode is not
generated when I factor it in the :P mode.  In the attached patch I
merged only the 32 and 64 bit modes into one pattern for the llwpcb
and slwpcb insns.

> Furthermore, as the insn has no output, I believe you have to use UNSPECV
> instead of UNSPEC.
>

Fixed.

> 2) slwpcb* - similarly, the builtins are VOID_FTYPE_VOID, when it is
> expected to be void *__builtin_ia32_slwpcb* (void).

Fixed.

> Again, I fail to see
> why 3 intrinsics are needed, just one would be enough.  In i386.md they have
> wrong patterns, as they set the registers it should be something like:
> (define_insn "lwp_slwpcb<mode>1"
>  [(set (match_operand:P 0 "register_operand" "=r")

I fixed the =r part.

>        (unspec [(const_int 0)] UNSPEC_SLWP_INTRINSIC))]
>  "TARGET_LWP"
>  "slwpcb\t%0"
>  [(set_attr "type" "lwp")
>   (set_attr "mode" "<MODE>")])
>

Same remark as above, about HI not generated.

> 3) lwpval*
>  { OPTION_MASK_ISA_LWP, CODE_FOR_lwp_lwpvalhi3, "__builtin_ia32_lwpval16", IX86_BUILTIN_LWPVAL16,  UNKNOWN,     (int) VOID_FTYPE_USHORT_UINT_USHORT },
>  { OPTION_MASK_ISA_LWP, CODE_FOR_lwp_lwpvalsi3, "__builtin_ia32_lwpval32", IX86_BUILTIN_LWPVAL64,  UNKNOWN,     (int) VOID_FTYPE_UINT_UINT_UINT },
>  { OPTION_MASK_ISA_LWP, CODE_FOR_lwp_lwpvaldi3, "__builtin_ia32_lwpval64", IX86_BUILTIN_LWPVAL64,  UNKNOWN,     (int) VOID_FTYPE_UINT64_UINT_UINT },
> typo on the second line, s/IX86_BUILTIN_LWPVAL64/IX86_BUILTIN_LWPVAL32/
> there.

Fixed.

> Also, I don't think we have anything like unsigned __int64
> type on Linux, guess you want to use int __attribute__((__mode__(__DI__)))
> instead.

I fixed this with what other insns are using in their intrinsics:
unsigned long long.

> 4) lwpins* is written to return char, what is the return value?
> rFlags.CF value after the insn?

Yes, rFlags.CF is what the lwpins* intrinsics are supposed to return.

> The insn pattern needs to be rewritten
> to make it clear that it sets the (reg:CC FLAGS_REG) to some unspec
> value.

I am proposing to use (clobber (reg:CC FLAGS_REG)) as some other insns
are doing.

> i386.c has similar typo (LWPINS64 instead of LWPINS32 for
> lwpins32).

Fixed.

> And it needs to arrange for the return value to be somehow set
> (define_expand that expands it to the actual lwpins insn plus setc insn?).
>

I am struggling with this one, could somebody help?

> 5) you shouldn't provide DI insns for !TARGET_64BIT, I don't think they are
> valid in 32-bit mode
>

Fixed.

> 6) for insns that you keep multiple versions of and don't use :P iterator,
> you should probably use :SWI248 mode iterator instead of duplicating
> the pattern 3 times.
>

Not done yet.

> Once the lwpval/lwpins stuff in lwpintrin.h is uncommented, you'll also need
> to further adjust my testsuite patch - you want to actually test the
> intrinsics that require immediate arguments, like they are tested in other
> intrin headers.
>

Not done yet.

Sebastian Pop
--
AMD / Open Source Compiler Engineering / GNU Tools