Re: [PATCH] Intrinsics for ADCX

[Michael Zolotukhin <michael.v.zolotukhin@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3045 bytes --]

Hi guys,
This patch generalizes recently commited addcarryx-intrinsic so that
it could be generated either via ADCX or common ADC instruction.
ADX-* tests are ok, bootstrap is passed.
Is it ok for trunk?

Changelog entry:
2012-08-09  Michael Zolotukhin  <michael.v.zolotukhin@intel.com>

        * config/i386/adxintrin.h: Remove guarding __ADX__ check.
        * config/i386/x86intrin.h: Likewise.
        * config/i386/i386.c (ix86_init_mmx_sse_builtins): Remove
        OPTION_MASK_ISA_ADX from needed options for
        __builtin_ia32_addcarryx_u32 and __builtin_ia32_addcarryx_u64.
        (ix86_expand_builtin): Use add<mode>3_carry in expanding of
        IX86_BUILTIN_ADDCARRYX32 and IX86_BUILTIN_ADDCARRYX64.

testsuite/Changelog entry:
2012-08-09  Michael Zolotukhin  <michael.v.zolotukhin@intel.com>

        * gcc.target/i386/adx-addxcarry32-3.c: New.
        * gcc.target/i386/adx-addxcarry64-3.c: New.


Thanks, Michael

On 1 August 2012 20:37, Kirill Yukhin <kirill.yukhin@gmail.com> wrote:
> Hi Richard,
>
>> Frankly I don't understand the point of these instructions
>> being added to the ISA at all.  I would have understood an
>> add-with-carry that did *not* modify the flags at all, but
>> two separate ones that modify C and O separately is just
>> downright strange.
> If there is only one carry in flight, they all are equivalent although
> ADOX is a little less useful in loops.
> If there are two carries in flight, that’s where the new instructions
> show their benefit, since they allow accumulation without destroying
> each other (see next comment).
> For any number of carries beyond two, you have to start saving
> restoring carry bits and it degenerates to the first case for some of
> them.
>
>> But to the point: I don't understand the point of having
>> this as a builtin.  Is the code generated by this builtin
>> any better than plain C?
> I think this is just like a practice to introduce new intrinsics for new insns.
> I doubt, that we may generate such things automatically:
> c1 = 0;
> c2 = 0;
> c1 = _adcx64( & res[i], src[i], src2[i], c1);
> c1 = _adcx64( & res[i+1], src[i+1], src2[i+1], c1);
> c2 = _adcx64( & res[i], src[i], src2[i], c2);
> c2 = _adcx64( & res[i+1], src[i+1], src2[i+1], c2);
>
>> And if you're going to have the builtin, why is this restricted
>> to adx anyway?  You obviously can produce the same results with
>> the good old fashioned adc instruction as well.
> We have one intrinsic for both ADCX/ADOX. So, we just picked up first
> one to use when exanding the built-in
>
>> Which begs the question of why you've got a separate pattern
>> for the adx anyway.  If the insn is so much better, it ought to
>> be used in the same pattern we use for adc now.
> I believe, we may introduce global variant of ADCX, which may be
> expanded into either of ADC/ADCX/ADOX on x86 and into analogs
> on the other ports.
>
> K


-- 
---
Best regards,
Michael V. Zolotukhin,
Software Engineer
Intel Corporation.

[-- Attachment #2: bdw-adx-5.gcc.patch --]
[-- Type: application/octet-stream, Size: 3000 bytes --]

diff --git a/gcc/config/i386/adxintrin.h b/gcc/config/i386/adxintrin.h
index 2e2a18b..a68566d 100644
--- a/gcc/config/i386/adxintrin.h
+++ b/gcc/config/i386/adxintrin.h
@@ -25,10 +25,6 @@
 # error "Never use <adxintrin.h> directly; include <x86intrin.h> instead."
 #endif
 
-#ifndef __ADX__
-# error "Flag-preserving add-carry instructions not enabled"
-#endif /* __ADX__ */
-
 #ifndef _ADXINTRIN_H_INCLUDED
 #define _ADXINTRIN_H_INCLUDED
 
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 17d4446..7a9e134 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -27968,9 +27968,9 @@ ix86_init_mmx_sse_builtins (void)
 	       INT_FTYPE_PULONGLONG, IX86_BUILTIN_RDSEED64_STEP);
 
   /* ADCX */
-  def_builtin (OPTION_MASK_ISA_ADX, "__builtin_ia32_addcarryx_u32",
+  def_builtin (0, "__builtin_ia32_addcarryx_u32",
 	       UCHAR_FTYPE_UCHAR_UINT_UINT_PUNSIGNED, IX86_BUILTIN_ADDCARRYX32);
-  def_builtin (OPTION_MASK_ISA_ADX && OPTION_MASK_ISA_64BIT,
+  def_builtin (OPTION_MASK_ISA_64BIT,
 	       "__builtin_ia32_addcarryx_u64",
 	       UCHAR_FTYPE_UCHAR_ULONGLONG_ULONGLONG_PULONGLONG,
 	       IX86_BUILTIN_ADDCARRYX64);
@@ -30343,12 +30343,12 @@ rdseed_step:
       return target;
 
     case IX86_BUILTIN_ADDCARRYX32:
-      icode = CODE_FOR_adcxsi3;
+      icode = TARGET_ADX ? CODE_FOR_adcxsi3 : CODE_FOR_addsi3_carry;
       mode0 = SImode;
       goto addcarryx;
 
     case IX86_BUILTIN_ADDCARRYX64:
-      icode = CODE_FOR_adcxdi3;
+      icode = TARGET_ADX ? CODE_FOR_adcxdi3 : CODE_FOR_adddi3_carry;
       mode0 = DImode;
 
 addcarryx:
diff --git a/gcc/config/i386/x86intrin.h b/gcc/config/i386/x86intrin.h
index dc5c58e..fae6491 100644
--- a/gcc/config/i386/x86intrin.h
+++ b/gcc/config/i386/x86intrin.h
@@ -105,8 +105,6 @@
 #include <prfchwintrin.h>
 #endif
 
-#ifdef __ADX__
 #include <adxintrin.h>
-#endif
 
 #endif /* _X86INTRIN_H_INCLUDED */
diff --git a/gcc/testsuite/gcc.target/i386/adx-addcarryx32-3.c b/gcc/testsuite/gcc.target/i386/adx-addcarryx32-3.c
new file mode 100644
index 0000000..0ed33a9
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/adx-addcarryx32-3.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-mno-adx -O2" } */
+/* { dg-final { scan-assembler "adcl" } } */
+
+#include <x86intrin.h>
+
+volatile unsigned char c;
+volatile unsigned int x, y;
+unsigned int *sum;
+
+void extern
+adx_test (void)
+{
+    c = _addcarryx_u32 (c, x, y, sum);
+}
diff --git a/gcc/testsuite/gcc.target/i386/adx-addcarryx64-3.c b/gcc/testsuite/gcc.target/i386/adx-addcarryx64-3.c
new file mode 100644
index 0000000..4bbf74b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/adx-addcarryx64-3.c
@@ -0,0 +1,15 @@
+/* { dg-do compile { target { ! ia32 } } } */
+/* { dg-options "-mno-adx -O2" } */
+/* { dg-final { scan-assembler "adcq" } } */
+
+#include <x86intrin.h>
+
+volatile unsigned char c;
+volatile unsigned long long x, y;
+unsigned long long *sum;
+
+void extern
+adx_test (void)
+{
+    c = _addcarryx_u64 (c, x, y, sum);
+}