Re: [PATCH, i386] RTM support

[Uros Bizjak <ubizjak at gmail dot com>]

On Tue, Mar 6, 2012 at 12:59 PM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote:
>> I'd suggest you generate local label in the expander and pass it to
>> insn RTX. This way, we can also reuse insn pattern later with eventual
>> different code label.
>
> Thanks! Done.
>
> New patch attached.
> Updated changelogs:
>
> ChangeLog:
> 2012-02-16 ?Kirill Yukhin <kirill.yukhin@intel.com>
>
> ? ? ? ?* common/config/i386/i386-common.c (OPTION_MASK_ISA_RTM_SET):
> ? ? ? ?New.
> ? ? ? ?(OPTION_MASK_ISA_RTM_UNSET): Ditto.
> ? ? ? ?(ix86_handle_option): Handle OPT_mrtm.
> ? ? ? ?* config.gcc (i[34567]86-*-*): Add rtmintrin.h and
> ? ? ? ?xtestintrin.h.
> ? ? ? ?(x86_64-*-*): Ditto.
> ? ? ? ?* i386-builtin-types.def (INT_FTYPE_VOID): New.
> ? ? ? ?* config/i386/i386-c.c (ix86_target_macros_internal): Define
> ? ? ? ?__RTM__ if needed.
> ? ? ? ?(ix86_target_string): Define -mrtm option.
> ? ? ? ?(PTA_RTM): New.
> ? ? ? ?(ix86_option_override_internal): Extend "cirei7-avx" with
> ? ? ? ?RTM option. Handle new option.
> ? ? ? ?(ix86_valid_target_attribute_inner_p): Add OPT_mrtm.
> ? ? ? ?(ix86_builtins): Add IX86_BUILTIN_XBEGIN, IX86_BUILTIN_XEND,
> ? ? ? ?IX86_BUILTIN_XTEST.
> ? ? ? ?(bdesc_special_args): Ditto.
> ? ? ? ?(ix86_init_mmx_sse_builtins): Add IX86_BUILTIN_XABORT.
> ? ? ? ?(ix86_expand_special_args_builtin): Handle new built-in type.
> ? ? ? ?(ix86_expand_builtin): Handle XABORT instruction.
> ? ? ? ?* config/i386/i386.h (TARGET_RTM): New.
> ? ? ? ?* config/i386/i386.md (UNSPECV_XBEGIN): New.
> ? ? ? ?(UNSPECV_XEND): Ditto.
> ? ? ? ?(UNSPECV_XABORT): Ditto.
> ? ? ? ?(UNSPECV_XTEST): Ditto.
> ? ? ? ?(xbegin): Ditto.
> ? ? ? ?(xbegin_1): Ditto.
> ? ? ? ?(xend): Ditto.
> ? ? ? ?(xabort): Ditto
> ? ? ? ?(xtest): Ditto.
> ? ? ? ?(xtest_1): Ditto.
> ? ? ? ?* config/i386/i386.opt (mrtm): New.
> ? ? ? ?* config/i386/immintrin.h: Include rtmintrin.h and
> ? ? ? ?xtestintrin.h.
> ? ? ? ?* config/i386/rtmintrin.h: New header.
> ? ? ? ?* config/i386/xtestintrin.h: Ditto.
>
> testsuite/ChangeLog:
> 2012-02-16 ?Kirill Yukhin <kirill.yukhin@intel.com>
>
> ? ? ? ?* gcc.target/i386/rtm-xabort-1.c: New.
> ? ? ? ?* gcc.target/i386/rtm-xbegin-1.c: Ditto.
> ? ? ? ?* gcc.target/i386/rtm-xend-1.c: Ditto.
> ? ? ? ?* gcc.target/i386/rtm-xtest-1.c: Ditto.
> ? ? ? ?* gcc.target/i386/sse-12.c: Test RTM intrinsics.
> ? ? ? ?* gcc.target/i386/sse-13.c: Ditto.
> ? ? ? ?* gcc.target/i386/sse-14.c: Ditto.
> ? ? ? ?* gcc.target/i386/sse-22.c: Ditto.
> ? ? ? ?* gcc.target/i386/sse-23.c: Ditto.
> ? ? ? ?* g++.dg/other/i386-2.C: Ditto.
> ? ? ? ?* g++.dg/other/i386-3.C: Ditto.
>
> Comments?

Technically OK, but let's wait for rth's comments about -mrtm option.

A few nits:

       break;
+    case INT_FTYPE_VOID:

Please add vertical space.

+(define_expand "xbegin"
+  [(set (match_operand:SI 0 "register_operand" "=a")
+   (unspec_volatile:SI [(match_dup 1)] UNSPECV_XBEGIN))]

Wrong indent.

+#ifdef __RTM__
+#include <rtmintrin.h>
+#endif
+
+#ifdef __RTM__
+#include <xtestintrin.h>
+#endif

Probably we don't need two separate #ifdefs.

+/* Copyright (C) 2011 Free Software Foundation, Inc.

Please update copyright year.

+/* { dg-options "-mrtm -O0" } */

+/* { dg-options "-mrtm -O0 -dp" } */

No need to pass -O0 (default) and -dp.

Thanks,
Uros.