Re: [PATCH] Enable SGX intrinsics

2017-01-11 Thread Uros Bizjak 
On Wed, Jan 11, 2017 at 8:59 PM, Koval, Julia  wrote:
> Hi, I rebased the patch onto latest trunk version and changed specification 
> according to ICC:
> _enclu_u32 (const int __L, size_t *__D)  -->  _enclu_u32 (const int __L, 
> size_t __D[])

I have committed the patch with additional testsuite changes and
removal of libgcc part. The later is wrong, since it changes order of
bits (please see the comment above the enum), and is generally not
needed, since it doesn't implement ABI addition, like SSE, AVX, or
similar.

2017-01-11  Julia Koval  

* common/config/i386/i386-common.c (OPTION_MASK_ISA_SGX_UNSET): New.
(OPTION_MASK_ISA_SGX_SET): New.
(ix86_handle_option): Handle OPT_msgx.
* config.gcc: Added sgxintrin.h.
* config/i386/driver-i386.c (host_detect_local_cpu): Detect sgx.
* config/i386/i386-c.c (ix86_target_macros_internal): Define __SGX__.
* config/i386/i386.c (ix86_target_string): Add -msgx.
(PTA_SGX): New.
(ix86_option_override_internal): Handle new options.
(ix86_valid_target_attribute_inner_p): Add sgx.
* config/i386/i386.h (TARGET_SGX, TARGET_SGX_P): New.
* config/i386/i386.opt: Add msgx.
* config/i386/sgxintrin.h: New file.
* config/i386/x86intrin.h: Add sgxintrin.h.

testsuite/ChangeLog:

2017-01-11  Julia Koval  
Uros Bizjak  

* gcc.target/i386/sgx.c New test.
* gcc.target/i386/sse-12.c: Add -msgx.
* 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.

Uros.
Index: common/config/i386/i386-common.c
===
--- common/config/i386/i386-common.c(revision 244335)
+++ common/config/i386/i386-common.c(working copy)
@@ -116,6 +116,7 @@ along with GCC; see the file COPYING3.  If not see
 #define OPTION_MASK_ISA_ABM_SET \
   (OPTION_MASK_ISA_ABM | OPTION_MASK_ISA_POPCNT)
 
+#define OPTION_MASK_ISA_SGX_SET OPTION_MASK_ISA_SGX
 #define OPTION_MASK_ISA_BMI_SET OPTION_MASK_ISA_BMI
 #define OPTION_MASK_ISA_BMI2_SET OPTION_MASK_ISA_BMI2
 #define OPTION_MASK_ISA_LZCNT_SET OPTION_MASK_ISA_LZCNT
@@ -214,6 +215,7 @@ along with GCC; see the file COPYING3.  If not see
 #define OPTION_MASK_ISA_SHA_UNSET OPTION_MASK_ISA_SHA
 #define OPTION_MASK_ISA_PCLMUL_UNSET OPTION_MASK_ISA_PCLMUL
 #define OPTION_MASK_ISA_ABM_UNSET OPTION_MASK_ISA_ABM
+#define OPTION_MASK_ISA_SGX_UNSET OPTION_MASK_ISA_SGX
 #define OPTION_MASK_ISA_BMI_UNSET OPTION_MASK_ISA_BMI
 #define OPTION_MASK_ISA_BMI2_UNSET OPTION_MASK_ISA_BMI2
 #define OPTION_MASK_ISA_LZCNT_UNSET OPTION_MASK_ISA_LZCNT
@@ -500,6 +502,19 @@ ix86_handle_option (struct gcc_options *opts,
}
   return true;
 
+case OPT_msgx:
+  if (value)
+   {
+ opts->x_ix86_isa_flags2 |= OPTION_MASK_ISA_SGX_SET;
+ opts->x_ix86_isa_flags2_explicit |= OPTION_MASK_ISA_SGX_SET;
+   }
+  else
+   {
+ opts->x_ix86_isa_flags2 &= ~OPTION_MASK_ISA_SGX_UNSET;
+ opts->x_ix86_isa_flags2_explicit |= OPTION_MASK_ISA_SGX_UNSET;
+   }
+  return true;
+
 case OPT_mavx512dq:
   if (value)
{
Index: config/i386/cpuid.h
===
--- config/i386/cpuid.h (revision 244335)
+++ config/i386/cpuid.h (working copy)
@@ -74,6 +74,7 @@
 /* Extended Features (%eax == 7) */
 /* %ebx */
 #define bit_FSGSBASE   (1 << 0)
+#define bit_SGX (1 << 2)
 #define bit_BMI(1 << 3)
 #define bit_HLE(1 << 4)
 #define bit_AVX2   (1 << 5)
Index: config/i386/driver-i386.c
===
--- config/i386/driver-i386.c   (revision 244335)
+++ config/i386/driver-i386.c   (working copy)
@@ -404,7 +404,7 @@ const char *host_detect_local_cpu (int argc, const
   unsigned int has_pclmul = 0, has_abm = 0, has_lwp = 0;
   unsigned int has_fma = 0, has_fma4 = 0, has_xop = 0;
   unsigned int has_bmi = 0, has_bmi2 = 0, has_tbm = 0, has_lzcnt = 0;
-  unsigned int has_hle = 0, has_rtm = 0;
+  unsigned int has_hle = 0, has_rtm = 0, has_sgx = 0;
   unsigned int has_rdrnd = 0, has_f16c = 0, has_fsgsbase = 0;
   unsigned int has_rdseed = 0, has_prfchw = 0, has_adx = 0;
   unsigned int has_osxsave = 0, has_fxsr = 0, has_xsave = 0, has_xsaveopt = 0;
@@ -480,6 +480,7 @@ const char *host_detect_local_cpu (int argc, const
   __cpuid_count (7, 0, eax, ebx, ecx, edx);
 
   has_bmi = ebx & bit_BMI;
+  has_sgx = ebx & bit_SGX;
   has_hle = ebx & bit_HLE;
   has_rtm = ebx & bit_RTM;
   has_avx2 = ebx & bit_AVX2;
@@ -993,6 +994,7 @@ const char *host_detect_local_cpu (int argc, const
   const char *fma4 = has_fma4 ? " -mfma4" : " -mno-fma4";
   const char *xop = has_xop ? " -mxop" : " -mno-xop";
   const char *bmi = has_bmi

RE: [PATCH] Enable SGX intrinsics

2017-01-11 Thread Koval, Julia 
Hi, I rebased the patch onto latest trunk version and changed specification 
according to ICC:
_enclu_u32 (const int __L, size_t *__D)  -->  _enclu_u32 (const int __L, size_t 
__D[])

The Changelogs remained the same:

gcc/
* common/config/i386/i386-common.c
   (OPTION_MASK_ISA_SGX_UNSET, OPTION_MASK_ISA_SGX_SET): New.
   (ix86_handle_option): Handle OPT_msgx.
* config.gcc: Added sgxintrin.h.
* config/i386/cpuid.h (bit_SGX): New.
* config/i386/driver-i386.c (host_detect_local_cpu): Detect sgx.
* config/i386/i386-c.c (ix86_target_macros_internal): Define __SGX__.
* config/i386/i386.c
   (ix86_target_string): Add -msgx.
   (PTA_SGX): New.
   (ix86_option_override_internal): Handle new options.
   (ix86_valid_target_attribute_inner_p): Add sgx.
* config/i386/i386.h (TARGET_SGX, TARGET_SGX_P): New.
* config/i386/i386.opt: Add msgx.
* config/i386/sgxintrin.h: New file.
* config/i386/x86intrin.h: Add sgxintrin.h.

gcc/testsuite/
* testsuite/gcc.target/i386/sgx.c New test.

libgcc/
config/i386/cpuinfo.c (get_available_features): Handle FEATURE_SGX.
config/i386/cpuinfo.h (FEATURE_SGX): New.

Thanks,
Julia

-Original Message-
From: Koval, Julia 
Sent: Wednesday, January 11, 2017 1:08 PM
To: 'Uros Bizjak' <ubiz...@gmail.com>
Cc: Andrew Senkevich <andrew.n.senkev...@gmail.com>; GCC Patches 
<gcc-patches@gcc.gnu.org>; vaalfr...@gmail.com; kirill.yuk...@gmail.com; Jakub 
Jelinek <ja...@redhat.com>
Subject: RE: [PATCH] Enable SGX intrinsics

Here is it.

gcc/
* common/config/i386/i386-common.c
   (OPTION_MASK_ISA_SGX_UNSET, OPTION_MASK_ISA_SGX_SET): New.
   (ix86_handle_option): Handle OPT_msgx.
* config.gcc: Added sgxintrin.h.
* config/i386/cpuid.h (bit_SGX): New.
* config/i386/driver-i386.c (host_detect_local_cpu): Detect sgx.
* config/i386/i386-c.c (ix86_target_macros_internal): Define __SGX__.
* config/i386/i386.c
   (ix86_target_string): Add -msgx.
   (PTA_SGX): New.
   (ix86_option_override_internal): Handle new options.
   (ix86_valid_target_attribute_inner_p): Add sgx.
* config/i386/i386.h (TARGET_SGX, TARGET_SGX_P): New.
* config/i386/i386.opt: Add msgx.
* config/i386/sgxintrin.h: New file.
* config/i386/x86intrin.h: Add sgxintrin.h.
* testsuite/gcc.target/i386/sgx.c New test

libgcc/
config/i386/cpuinfo.c (get_available_features): Handle FEATURE_SGX.
config/i386/cpuinfo.h (FEATURE_SGX): New.

Thanks,
Julia

-Original Message-
From: Uros Bizjak [mailto:ubiz...@gmail.com] 
Sent: Wednesday, January 11, 2017 1:02 PM
To: Koval, Julia <julia.ko...@intel.com>
Cc: Andrew Senkevich <andrew.n.senkev...@gmail.com>; GCC Patches 
<gcc-patches@gcc.gnu.org>; vaalfr...@gmail.com; kirill.yuk...@gmail.com; Jakub 
Jelinek <ja...@redhat.com>
Subject: Re: [PATCH] Enable SGX intrinsics

On Wed, Jan 11, 2017 at 12:40 PM, Koval, Julia <julia.ko...@intel.com> wrote:
> Ok, fixed it. Can you please commit it for me, cause I don't have rights to 
> commit?

OK, but please send me updated ChangeLogs.

Uros.

> Thanks,
> Julia
>
> -Original Message-
> From: Uros Bizjak [mailto:ubiz...@gmail.com]
> Sent: Wednesday, January 11, 2017 12:11 PM
> To: Koval, Julia <julia.ko...@intel.com>
> Cc: Andrew Senkevich <andrew.n.senkev...@gmail.com>; GCC Patches 
> <gcc-patches@gcc.gnu.org>; vaalfr...@gmail.com; kirill.yuk...@gmail.com; 
> Jakub Jelinek <ja...@redhat.com>
> Subject: Re: [PATCH] Enable SGX intrinsics
>
> On Wed, Jan 11, 2017 at 11:31 AM, Koval, Julia <julia.ko...@intel.com> wrote:
>> Ok. I fixed the enum formatting and the enums remain internal.
>
> @@ -7023,7 +7029,6 @@ ix86_can_inline_p (tree caller, tree callee)
>bool ret = false;
>tree caller_tree = DECL_FUNCTION_SPECIFIC_TARGET (caller);
>tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (callee);
> -
>/* If callee has no option attributes, then it is ok to inline.  */
>if (!callee_tree)
>  ret = true;
>
>
> No need for the above whitespace change.
>
> OK for mainline with the above part reverted.
>
> Thanks,
> Uros.


0001-SGX-intrinsiks.patch
Description: 0001-SGX-intrinsiks.patch

RE: [PATCH] Enable SGX intrinsics

2017-01-11 Thread Koval, Julia 
Here is it.

gcc/
* common/config/i386/i386-common.c
   (OPTION_MASK_ISA_SGX_UNSET, OPTION_MASK_ISA_SGX_SET): New.
   (ix86_handle_option): Handle OPT_msgx.
* config.gcc: Added sgxintrin.h.
* config/i386/cpuid.h (bit_SGX): New.
* config/i386/driver-i386.c (host_detect_local_cpu): Detect sgx.
* config/i386/i386-c.c (ix86_target_macros_internal): Define __SGX__.
* config/i386/i386.c
   (ix86_target_string): Add -msgx.
   (PTA_SGX): New.
   (ix86_option_override_internal): Handle new options.
   (ix86_valid_target_attribute_inner_p): Add sgx.
* config/i386/i386.h (TARGET_SGX, TARGET_SGX_P): New.
* config/i386/i386.opt: Add msgx.
* config/i386/sgxintrin.h: New file.
* config/i386/x86intrin.h: Add sgxintrin.h.
* testsuite/gcc.target/i386/sgx.c New test

libgcc/
config/i386/cpuinfo.c (get_available_features): Handle FEATURE_SGX.
config/i386/cpuinfo.h (FEATURE_SGX): New.

Thanks,
Julia

-Original Message-
From: Uros Bizjak [mailto:ubiz...@gmail.com] 
Sent: Wednesday, January 11, 2017 1:02 PM
To: Koval, Julia <julia.ko...@intel.com>
Cc: Andrew Senkevich <andrew.n.senkev...@gmail.com>; GCC Patches 
<gcc-patches@gcc.gnu.org>; vaalfr...@gmail.com; kirill.yuk...@gmail.com; Jakub 
Jelinek <ja...@redhat.com>
Subject: Re: [PATCH] Enable SGX intrinsics

On Wed, Jan 11, 2017 at 12:40 PM, Koval, Julia <julia.ko...@intel.com> wrote:
> Ok, fixed it. Can you please commit it for me, cause I don't have rights to 
> commit?

OK, but please send me updated ChangeLogs.

Uros.

> Thanks,
> Julia
>
> -Original Message-
> From: Uros Bizjak [mailto:ubiz...@gmail.com]
> Sent: Wednesday, January 11, 2017 12:11 PM
> To: Koval, Julia <julia.ko...@intel.com>
> Cc: Andrew Senkevich <andrew.n.senkev...@gmail.com>; GCC Patches 
> <gcc-patches@gcc.gnu.org>; vaalfr...@gmail.com; kirill.yuk...@gmail.com; 
> Jakub Jelinek <ja...@redhat.com>
> Subject: Re: [PATCH] Enable SGX intrinsics
>
> On Wed, Jan 11, 2017 at 11:31 AM, Koval, Julia <julia.ko...@intel.com> wrote:
>> Ok. I fixed the enum formatting and the enums remain internal.
>
> @@ -7023,7 +7029,6 @@ ix86_can_inline_p (tree caller, tree callee)
>bool ret = false;
>tree caller_tree = DECL_FUNCTION_SPECIFIC_TARGET (caller);
>tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (callee);
> -
>/* If callee has no option attributes, then it is ok to inline.  */
>if (!callee_tree)
>  ret = true;
>
>
> No need for the above whitespace change.
>
> OK for mainline with the above part reverted.
>
> Thanks,
> Uros.

Re: [PATCH] Enable SGX intrinsics

2017-01-11 Thread Uros Bizjak 
On Wed, Jan 11, 2017 at 12:40 PM, Koval, Julia <julia.ko...@intel.com> wrote:
> Ok, fixed it. Can you please commit it for me, cause I don't have rights to 
> commit?

OK, but please send me updated ChangeLogs.

Uros.

> Thanks,
> Julia
>
> -Original Message-
> From: Uros Bizjak [mailto:ubiz...@gmail.com]
> Sent: Wednesday, January 11, 2017 12:11 PM
> To: Koval, Julia <julia.ko...@intel.com>
> Cc: Andrew Senkevich <andrew.n.senkev...@gmail.com>; GCC Patches 
> <gcc-patches@gcc.gnu.org>; vaalfr...@gmail.com; kirill.yuk...@gmail.com; 
> Jakub Jelinek <ja...@redhat.com>
> Subject: Re: [PATCH] Enable SGX intrinsics
>
> On Wed, Jan 11, 2017 at 11:31 AM, Koval, Julia <julia.ko...@intel.com> wrote:
>> Ok. I fixed the enum formatting and the enums remain internal.
>
> @@ -7023,7 +7029,6 @@ ix86_can_inline_p (tree caller, tree callee)
>bool ret = false;
>tree caller_tree = DECL_FUNCTION_SPECIFIC_TARGET (caller);
>tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (callee);
> -
>/* If callee has no option attributes, then it is ok to inline.  */
>if (!callee_tree)
>  ret = true;
>
>
> No need for the above whitespace change.
>
> OK for mainline with the above part reverted.
>
> Thanks,
> Uros.

RE: [PATCH] Enable SGX intrinsics

2017-01-11 Thread Koval, Julia 
Ok, fixed it. Can you please commit it for me, cause I don't have rights to 
commit?

Thanks,
Julia

-Original Message-
From: Uros Bizjak [mailto:ubiz...@gmail.com] 
Sent: Wednesday, January 11, 2017 12:11 PM
To: Koval, Julia <julia.ko...@intel.com>
Cc: Andrew Senkevich <andrew.n.senkev...@gmail.com>; GCC Patches 
<gcc-patches@gcc.gnu.org>; vaalfr...@gmail.com; kirill.yuk...@gmail.com; Jakub 
Jelinek <ja...@redhat.com>
Subject: Re: [PATCH] Enable SGX intrinsics

On Wed, Jan 11, 2017 at 11:31 AM, Koval, Julia <julia.ko...@intel.com> wrote:
> Ok. I fixed the enum formatting and the enums remain internal.

@@ -7023,7 +7029,6 @@ ix86_can_inline_p (tree caller, tree callee)
   bool ret = false;
   tree caller_tree = DECL_FUNCTION_SPECIFIC_TARGET (caller);
   tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (callee);
-
   /* If callee has no option attributes, then it is ok to inline.  */
   if (!callee_tree)
 ret = true;


No need for the above whitespace change.

OK for mainline with the above part reverted.

Thanks,
Uros.


0001-Enable-SGX.PATCH
Description: 0001-Enable-SGX.PATCH

Re: [PATCH] Enable SGX intrinsics

2017-01-11 Thread Uros Bizjak 
On Wed, Jan 11, 2017 at 11:31 AM, Koval, Julia  wrote:
> Ok. I fixed the enum formatting and the enums remain internal.

@@ -7023,7 +7029,6 @@ ix86_can_inline_p (tree caller, tree callee)
   bool ret = false;
   tree caller_tree = DECL_FUNCTION_SPECIFIC_TARGET (caller);
   tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (callee);
-
   /* If callee has no option attributes, then it is ok to inline.  */
   if (!callee_tree)
 ret = true;


No need for the above whitespace change.

OK for mainline with the above part reverted.

Thanks,
Uros.

Re: [PATCH] Enable SGX intrinsics

2017-01-11 Thread Jakub Jelinek 
On Wed, Jan 11, 2017 at 10:31:33AM +, Koval, Julia wrote:
> Ok. I fixed the enum formatting and the enums remain internal.

No further objections from me, if Uros acks it, check it in.

> > Sure.  Plus it depends on if users of the APIs should just write the 
> > operands on their own as numbers, or as __SGX_E*, or as E*.
> > In the first case the patch sans formatting is reasonable, in the second 
> > case the enums should be moved to file scope, in the last case we have to 
> > live with the namespace pollution.
> > The pdf you've referenced in the thread doesn't list the _encls_u32 and
> > _enclu_u32 intrinsics, so I think it depends on what ICC does (if it has 
> > been shipped with such a support already, or on coordination with ICC if 
> > not).
> 
> Jakub, it is in accordance with ICC.
> So the first case will be used.

Jakub

RE: [PATCH] Enable SGX intrinsics

2017-01-11 Thread Koval, Julia 
Ok. I fixed the enum formatting and the enums remain internal.

-Julia

-Original Message-
From: Andrew Senkevich [mailto:andrew.n.senkev...@gmail.com] 
Sent: Tuesday, January 10, 2017 5:48 PM
To: Uros Bizjak <ubiz...@gmail.com>
Cc: Koval, Julia <julia.ko...@intel.com>; GCC Patches 
<gcc-patches@gcc.gnu.org>; vaalfr...@gmail.com
Subject: Re: [PATCH] Enable SGX intrinsics

On Fri, Dec 30, 2016 at 03:37:14PM +0100, Uros Bizjak wrote:
>> As suggested in [1], you should write multi-line enums like:
>>
>> enum foo
>> {
>>   a = ...
>>   b = ...
>> }
>
> Sure.  Plus it depends on if users of the APIs should just write the operands 
> on their own as numbers, or as __SGX_E*, or as E*.
> In the first case the patch sans formatting is reasonable, in the second case 
> the enums should be moved to file scope, in the last case we have to live 
> with the namespace pollution.
> The pdf you've referenced in the thread doesn't list the _encls_u32 and
> _enclu_u32 intrinsics, so I think it depends on what ICC does (if it has been 
> shipped with such a support already, or on coordination with ICC if not).

Jakub, it is in accordance with ICC.
So the first case will be used.


--
WBR,
Andrew


0001-Enable-SGX.PATCH
Description: 0001-Enable-SGX.PATCH

Re: [PATCH] Enable SGX intrinsics

2017-01-10 Thread Andrew Senkevich 
On Fri, Dec 30, 2016 at 03:37:14PM +0100, Uros Bizjak wrote:
>> As suggested in [1], you should write multi-line enums like:
>>
>> enum foo
>> {
>>   a = ...
>>   b = ...
>> }
>
> Sure.  Plus it depends on if users of the APIs should just write the operands 
> on their own as numbers, or as __SGX_E*, or as E*.
> In the first case the patch sans formatting is reasonable, in the second case 
> the enums should be moved to file scope, in the last case we have to live 
> with the namespace pollution.
> The pdf you've referenced in the thread doesn't list the _encls_u32 and
> _enclu_u32 intrinsics, so I think it depends on what ICC does (if it has been 
> shipped with such a support already, or on coordination with ICC if not).

Jakub, it is in accordance with ICC.
So the first case will be used.


--
WBR,
Andrew

Re: [PATCH] Enable SGX intrinsics

2016-12-30 Thread Jakub Jelinek 
On Fri, Dec 30, 2016 at 03:37:14PM +0100, Uros Bizjak wrote:
> As suggested in [1], you should write multi-line enums like:
> 
> enum foo
> {
>   a = ...
>   b = ...
> }

Sure.  Plus it depends on if users of the APIs should just write the
operands on their own as numbers, or as __SGX_E*, or as E*.
In the first case the patch sans formatting is reasonable, in the second
case the enums should be moved to file scope, in the last case we have to
live with the namespace pollution.
The pdf you've referenced in the thread doesn't list the _encls_u32 and
_enclu_u32 intrinsics, so I think it depends on what ICC does (if it has
been shipped with such a support already, or on coordination with ICC if not).

Jakub

Re: [PATCH] Enable SGX intrinsics

2016-12-30 Thread Uros Bizjak 
On Fri, Dec 30, 2016 at 3:17 PM, Koval, Julia  wrote:
> Thank you for your comments, how about this patch? Enums are not part of the 
> intrinsic ABI, they are just meaningful names for constants, taken from 
> reference doc.
>
> gcc/
> * common/config/i386/i386-common.c
>(OPTION_MASK_ISA_SGX_UNSET, OPTION_MASK_ISA_SGX_SET): New.
>(ix86_handle_option): Handle OPT_msgx.
> * config.gcc: Added sgxintrin.h.
> * config/i386/cpuid.h (bit_SGX): New.
> * config/i386/driver-i386.c (host_detect_local_cpu): Detect sgx.
> * config/i386/i386-c.c (ix86_target_macros_internal): Define __SGX__.
> * config/i386/i386.c
>(ix86_target_string): Add -msgx.
>(PTA_SGX): New.
>(ix86_option_override_internal): Handle new options.
>(ix86_valid_target_attribute_inner_p): Add sgx.
> * config/i386/i386.h (TARGET_SGX, TARGET_SGX_P): New.
> * config/i386/i386.opt: Add msgx.
> * config/i386/sgxintrin.h: New file.
> * config/i386/x86intrin.h: Add sgxintrin.h.
> * testsuite/gcc.target/i386/sgx.c New test
>
> libgcc/
> config/i386/cpuinfo.c (get_available_features): Handle FEATURE_SGX.
> config/i386/cpuinfo.h (FEATURE_SGX): New.

As suggested in [1], you should write multi-line enums like:

enum foo
{
  a = ...
  b = ...
}

OK with the above change(s), but please wait for Jakub, if he has some
more comments.

[1] https://www.gnu.org/prep/standards/standards.html

Thanks,
Uros.

RE: [PATCH] Enable SGX intrinsics

2016-12-30 Thread Koval, Julia 
Thank you for your comments, how about this patch? Enums are not part of the 
intrinsic ABI, they are just meaningful names for constants, taken from 
reference doc.

gcc/
* common/config/i386/i386-common.c
   (OPTION_MASK_ISA_SGX_UNSET, OPTION_MASK_ISA_SGX_SET): New.
   (ix86_handle_option): Handle OPT_msgx.
* config.gcc: Added sgxintrin.h.
* config/i386/cpuid.h (bit_SGX): New.
* config/i386/driver-i386.c (host_detect_local_cpu): Detect sgx.
* config/i386/i386-c.c (ix86_target_macros_internal): Define __SGX__.
* config/i386/i386.c
   (ix86_target_string): Add -msgx.
   (PTA_SGX): New.
   (ix86_option_override_internal): Handle new options.
   (ix86_valid_target_attribute_inner_p): Add sgx.
* config/i386/i386.h (TARGET_SGX, TARGET_SGX_P): New.
* config/i386/i386.opt: Add msgx.
* config/i386/sgxintrin.h: New file.
* config/i386/x86intrin.h: Add sgxintrin.h.
* testsuite/gcc.target/i386/sgx.c New test

libgcc/
config/i386/cpuinfo.c (get_available_features): Handle FEATURE_SGX.
config/i386/cpuinfo.h (FEATURE_SGX): New.

BR,
Julia

-Original Message-
From: Jakub Jelinek [mailto:ja...@redhat.com] 
Sent: Friday, December 30, 2016 10:19 AM
To: Uros Bizjak <ubiz...@gmail.com>
Cc: Koval, Julia <julia.ko...@intel.com>; gcc-patches@gcc.gnu.org; 
vaalfr...@gmail.com; Senkevich, Andrew <andrew.senkev...@intel.com>
Subject: Re: [PATCH] Enable SGX intrinsics

On Fri, Dec 30, 2016 at 09:25:49AM +0100, Uros Bizjak wrote:
> On Thu, Dec 29, 2016 at 10:50 AM, Koval, Julia <julia.ko...@intel.com> wrote:
> > Hi,
> >
> > This patch enables Intel SGX instructions (Reference: 
> > https://software.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf
> >  page 4478 in pdf and 3D 41-1 in page numbers) Ok for trunk?
> 
> I don't like asm macros, but since we can tolerate similar 
> implementation of cpuid, we can also tolerate encls/enclu.
> 
> One genreal remark:
> 
> +#define macro_encls_bc(leaf, b, c, retval) \
> +  __asm__ __volatile__ ("encls\n\t" \
> +   : "=a" (retval) \
> +   : "a" (leaf), "b" (b), "c" (c))
> 
> These internal macros are user-visible, so please uglify them with a 
> double underscore, like
> 
> __encls_bc
> 
> to put them into internal namespace. IMO, there is no need to use 
> "macro" prefix.

I see other issues:

+extern __inline int
+__attribute__((__gnu_inline__, __always_inline__, __artificial__))
+_encls_u32 (const int __leaf, size_t *data) {
+  enum encls_type type = (enum encls_type)__leaf;
+  int retval = 0;
+  if (!__builtin_constant_p (type))
+  {
+macro_encls_generic (__leaf, data[0], data[1], data[2], retval);
+  }
+  else

1) some parameters and variable names not uglified in inline functions, data, 
type, retval in this case; using __data etc. or __L, __D, __T, __R would be 
better; look at other intrin files for what identifiers they are using
2) the indentation is wrong, { should after if/else should be indented
2 more columns to the right, so column 4 in this case, and the body of the 
block 2 further positions.
3) For a single line body there should be no {}s around, so put the macro 
without the {}s.

Is:
enum encls_type {ECREATE, EADD, EINIT, EREMOVE, EDBGRD, EDBGWR, EEXTEND,
 ELDB, ELDU, EBLOCK, EPA, EWB, ETRACK, EAUG, EMODPR, EMODT};

enum enclu_type {EREPORT, EGETKEY, EENTER, ERESUME, EEXIT, EACCEPT, EMODPE,
 EACCEPTCOPY};
really part of the official sgxintrin.h ABI?
4) the enum names are not uglified
5) the enumerators use namespace reserved for errno.h extensions, see
   http://pubs.opengroup.org/onlinepubs/007904975/functions/xsh_chap02_02.html
Header  Prefix
   E[0-9], E[A-Z]
Especially because they aren't used for errors in this case, it is a 
particularly bad choice; and, because the header is unconditionally included in 
x86intrin.h, this won't affect just users that want SGX, but all users of 
x86intrin.h, which means huge amount of code in the wild.

Jakub


0001-Enable-SGX.PATCH
Description: 0001-Enable-SGX.PATCH

Re: [PATCH] Enable SGX intrinsics

2016-12-30 Thread Jakub Jelinek 
On Fri, Dec 30, 2016 at 09:25:49AM +0100, Uros Bizjak wrote:
> On Thu, Dec 29, 2016 at 10:50 AM, Koval, Julia  wrote:
> > Hi,
> >
> > This patch enables Intel SGX instructions (Reference: 
> > https://software.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf
> >  page 4478 in pdf and 3D 41-1 in page numbers) Ok for trunk?
> 
> I don't like asm macros, but since we can tolerate similar
> implementation of cpuid, we can also tolerate encls/enclu.
> 
> One genreal remark:
> 
> +#define macro_encls_bc(leaf, b, c, retval) \
> +  __asm__ __volatile__ ("encls\n\t" \
> +   : "=a" (retval) \
> +   : "a" (leaf), "b" (b), "c" (c))
> 
> These internal macros are user-visible, so please uglify them with a
> double underscore, like
> 
> __encls_bc
> 
> to put them into internal namespace. IMO, there is no need to use
> "macro" prefix.

I see other issues:

+extern __inline int
+__attribute__((__gnu_inline__, __always_inline__, __artificial__))
+_encls_u32 (const int __leaf, size_t *data)
+{
+  enum encls_type type = (enum encls_type)__leaf;
+  int retval = 0;
+  if (!__builtin_constant_p (type))
+  {
+macro_encls_generic (__leaf, data[0], data[1], data[2], retval);
+  }
+  else

1) some parameters and variable names not uglified in inline functions,
data, type, retval in this case; using __data etc. or __L, __D, __T, __R
would be better; look at other intrin files for what identifiers they are
using
2) the indentation is wrong, { should after if/else should be indented
2 more columns to the right, so column 4 in this case, and the body of the
block 2 further positions.
3) For a single line body there should be no {}s around, so put the macro
without the {}s.

Is:
enum encls_type {ECREATE, EADD, EINIT, EREMOVE, EDBGRD, EDBGWR, EEXTEND,
 ELDB, ELDU, EBLOCK, EPA, EWB, ETRACK, EAUG, EMODPR, EMODT};

enum enclu_type {EREPORT, EGETKEY, EENTER, ERESUME, EEXIT, EACCEPT, EMODPE,
 EACCEPTCOPY};
really part of the official sgxintrin.h ABI?
4) the enum names are not uglified
5) the enumerators use namespace reserved for errno.h extensions, see
   http://pubs.opengroup.org/onlinepubs/007904975/functions/xsh_chap02_02.html
Header  Prefix
   E[0-9], E[A-Z]
Especially because they aren't used for errors in this case, it is a
particularly bad choice; and, because the header is unconditionally
included in x86intrin.h, this won't affect just users that want SGX, but all
users of x86intrin.h, which means huge amount of code in the wild.

Jakub

Re: [PATCH] Enable SGX intrinsics

2016-12-30 Thread Uros Bizjak 
On Thu, Dec 29, 2016 at 10:50 AM, Koval, Julia  wrote:
> Hi,
>
> This patch enables Intel SGX instructions (Reference: 
> https://software.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf
>  page 4478 in pdf and 3D 41-1 in page numbers) Ok for trunk?

I don't like asm macros, but since we can tolerate similar
implementation of cpuid, we can also tolerate encls/enclu.

One genreal remark:

+#define macro_encls_bc(leaf, b, c, retval) \
+  __asm__ __volatile__ ("encls\n\t" \
+   : "=a" (retval) \
+   : "a" (leaf), "b" (b), "c" (c))

These internal macros are user-visible, so please uglify them with a
double underscore, like

__encls_bc

to put them into internal namespace. IMO, there is no need to use
"macro" prefix.

Uros.

Re: [PATCH] Enable SGX intrinsics

2016-12-29 Thread Yulia Koval 
Sorry, didn't include changelog. Here is it:

gcc/
* common/config/i386/i386-common.c
   (OPTION_MASK_ISA_SGX_UNSET, OPTION_MASK_ISA_SGX_SET): New.
   (ix86_handle_option): Handle OPT_msgx.
* config.gcc: Added sgxintrin.h.
* config/i386/cpuid.h (bit_SGX): New.
* config/i386/driver-i386.c (host_detect_local_cpu): Detect sgx.
* config/i386/i386-c.c (ix86_target_macros_internal): Define __SGX__.
* config/i386/i386.c
   (ix86_target_string): Add -msgx.
   (PTA_SGX): New.
   (ix86_option_override_internal): Handle new options.
   (ix86_valid_target_attribute_inner_p): Add sgx.
* config/i386/i386.h (TARGET_SGX, TARGET_SGX_P): New.
* config/i386/i386.opt: Add msgx.
* config/i386/sgxintrin.h: New file.
* config/i386/x86intrin.h: Add sgxintrin.h.
* testsuite/gcc.target/i386/sgx.c New test

libgcc/
config/i386/cpuinfo.c (get_available_features): Handle FEATURE_SGX.
config/i386/cpuinfo.h (FEATURE_SGX): New.

On Thu, Dec 29, 2016 at 10:50 AM, Koval, Julia  wrote:
> Hi,
>
> This patch enables Intel SGX instructions (Reference: 
> https://software.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf
>  page 4478 in pdf and 3D 41-1 in page numbers) Ok for trunk?
>
> Thanks,
> Julia

[PATCH] Enable SGX intrinsics

2016-12-29 Thread Koval, Julia 
Hi,

This patch enables Intel SGX instructions (Reference: 
https://software.intel.com/sites/default/files/managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf
 page 4478 in pdf and 3D 41-1 in page numbers) Ok for trunk?

Thanks,
Julia


0001-Enable-SGX.patch
Description: 0001-Enable-SGX.patch