| Message ID | 20120520170426.GA7774@intel.com |
|---|---|
| State | New |
| Headers | show |
On Sun, May 20, 2012 at 10:04:26AM -0700, H.J. Lu wrote: > rdrand<mode>_1 must be marked with unspec_volatile since it returns > a different value every time. OK for trunk, 4.7 and 4.6? A testcase for this would be nice (runtime is not possible, since the RNG in theory could return the same value twice, but scanning assembly for a particular number of the rdrand insns would be nice). > PR target/53416 > * config/i386/i386.md (UNSPEC_RDRAND): Renamed to ... > (UNSPECV_RDRAND): This. > (rdrand<mode>_1): Updated. Jakub
On Sun, May 20, 2012 at 10:19 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Sun, May 20, 2012 at 10:04:26AM -0700, H.J. Lu wrote: >> rdrand<mode>_1 must be marked with unspec_volatile since it returns >> a different value every time. OK for trunk, 4.7 and 4.6? > > A testcase for this would be nice (runtime is not possible, since the > RNG in theory could return the same value twice, but scanning assembly > for a particular number of the rdrand insns would be nice). > For unsigned int number = 0; volatile int result = 0; for (register int i = 0; i < 4; ++i) { result = _rdrand32_step(&number); printf("%d: %d\n", result, number); } the issue isn't about number of rdrand insns. As long as it isn't hoisted out of the loop, one rdrand insn is OK. I don't know how to scan for inside or outside of loop.
On Sun, May 20, 2012 at 10:37:13AM -0700, H.J. Lu wrote: > On Sun, May 20, 2012 at 10:19 AM, Jakub Jelinek <jakub@redhat.com> wrote: > > On Sun, May 20, 2012 at 10:04:26AM -0700, H.J. Lu wrote: > >> rdrand<mode>_1 must be marked with unspec_volatile since it returns > >> a different value every time. OK for trunk, 4.7 and 4.6? > > > > A testcase for this would be nice (runtime is not possible, since the > > RNG in theory could return the same value twice, but scanning assembly > > for a particular number of the rdrand insns would be nice). > > > > For > > unsigned int number = 0; > volatile int result = 0; > > for (register int i = 0; i < 4; ++i) { > result = _rdrand32_step(&number); > printf("%d: %d\n", result, number); > } Try it without the loop, unroll it by hand, see if without the patch the rdrand insns are still CSEd together? Jakub
On Sun, May 20, 2012 at 11:15 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Sun, May 20, 2012 at 10:37:13AM -0700, H.J. Lu wrote: >> On Sun, May 20, 2012 at 10:19 AM, Jakub Jelinek <jakub@redhat.com> wrote: >> > On Sun, May 20, 2012 at 10:04:26AM -0700, H.J. Lu wrote: >> >> rdrand<mode>_1 must be marked with unspec_volatile since it returns >> >> a different value every time. OK for trunk, 4.7 and 4.6? >> > >> > A testcase for this would be nice (runtime is not possible, since the >> > RNG in theory could return the same value twice, but scanning assembly >> > for a particular number of the rdrand insns would be nice). >> > >> >> For >> >> unsigned int number = 0; >> volatile int result = 0; >> >> for (register int i = 0; i < 4; ++i) { >> result = _rdrand32_step(&number); >> printf("%d: %d\n", result, number); >> } > > Try it without the loop, unroll it by hand, see if without the patch > the rdrand insns are still CSEd together? > It doesn't: [hjl@gnu-ivb-1 tmp]$ cat x.c #include <stdio.h> int main(int argc, char **argv) { unsigned int number = 0; volatile int result = 0; result = __builtin_ia32_rdrand32_step (&number); printf("%d: %d\n", result, number); result = __builtin_ia32_rdrand32_step (&number); printf("%d: %d\n", result, number); result = __builtin_ia32_rdrand32_step (&number); printf("%d: %d\n", result, number); result = __builtin_ia32_rdrand32_step (&number); printf("%d: %d\n", result, number); return 0; } [hjl@gnu-ivb-1 tmp]$ /export/gnu/import/git/gcc-regression/master/187369/usr/bin/gcc -mrdrnd -O3 -S x.c [hjl@gnu-ivb-1 tmp]$ grep rdrand x.s rdrand %ebx rdrand %eax rdrand %eax rdrand %eax [hjl@gnu-ivb-1 tmp]$
On Sun, May 20, 2012 at 11:37 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Sun, May 20, 2012 at 11:15 AM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Sun, May 20, 2012 at 10:37:13AM -0700, H.J. Lu wrote: >>> On Sun, May 20, 2012 at 10:19 AM, Jakub Jelinek <jakub@redhat.com> wrote: >>> > On Sun, May 20, 2012 at 10:04:26AM -0700, H.J. Lu wrote: >>> >> rdrand<mode>_1 must be marked with unspec_volatile since it returns >>> >> a different value every time. OK for trunk, 4.7 and 4.6? >>> > >>> > A testcase for this would be nice (runtime is not possible, since the >>> > RNG in theory could return the same value twice, but scanning assembly >>> > for a particular number of the rdrand insns would be nice). >>> > >>> >>> For >>> >>> unsigned int number = 0; >>> volatile int result = 0; >>> >>> for (register int i = 0; i < 4; ++i) { >>> result = _rdrand32_step(&number); >>> printf("%d: %d\n", result, number); >>> } >> >> Try it without the loop, unroll it by hand, see if without the patch >> the rdrand insns are still CSEd together? >> > > It doesn't: > > [hjl@gnu-ivb-1 tmp]$ cat x.c > #include <stdio.h> > > int > main(int argc, char **argv) > { > unsigned int number = 0; > volatile int result = 0; > > result = __builtin_ia32_rdrand32_step (&number); > printf("%d: %d\n", result, number); > result = __builtin_ia32_rdrand32_step (&number); > printf("%d: %d\n", result, number); > result = __builtin_ia32_rdrand32_step (&number); > printf("%d: %d\n", result, number); > result = __builtin_ia32_rdrand32_step (&number); > printf("%d: %d\n", result, number); > return 0; > } > [hjl@gnu-ivb-1 tmp]$ > /export/gnu/import/git/gcc-regression/master/187369/usr/bin/gcc > -mrdrnd -O3 -S x.c > [hjl@gnu-ivb-1 tmp]$ grep rdrand x.s > rdrand %ebx > rdrand %eax > rdrand %eax > rdrand %eax > [hjl@gnu-ivb-1 tmp]$ Try: #include <stdio.h> int main(int argc, char **argv) { unsigned int number = 0; int result0, result1, result2, result3; result0 = __builtin_ia32_rdrand32_step (&number); result1 = __builtin_ia32_rdrand32_step (&number); result2 = __builtin_ia32_rdrand32_step (&number); result3 = __builtin_ia32_rdrand32_step (&number); printf("%d: %d\n", result0, number); printf("%d: %d\n", result1, number); printf("%d: %d\n", result2, number); printf("%d: %d\n", result3, number); return 0; } Which I Know for a fact fails before the patch: pinskia@server:~$ grep rdrand t.s rdrand %edx pinskia@server:~$ Thanks, Andrew Pinski > > > -- > H.J.
On Sun, May 20, 2012 at 8:43 PM, Andrew Pinski <pinskia@gmail.com> wrote: > #include <stdio.h> > > int > main(int argc, char **argv) > { > unsigned int number = 0; > int result0, result1, result2, result3; > > result0 = __builtin_ia32_rdrand32_step (&number); > result1 = __builtin_ia32_rdrand32_step (&number); > result2 = __builtin_ia32_rdrand32_step (&number); > result3 = __builtin_ia32_rdrand32_step (&number); > printf("%d: %d\n", result0, number); > printf("%d: %d\n", result1, number); > printf("%d: %d\n", result2, number); > printf("%d: %d\n", result3, number); > return 0; > } > int test (void) { unsigned int number = 0; int result0, result1, result2, result3; result0 = __builtin_ia32_rdrand32_step (&number); result1 = __builtin_ia32_rdrand32_step (&number); result2 = __builtin_ia32_rdrand32_step (&number); result3 = __builtin_ia32_rdrand32_step (&number); return result0 + result1 +result2 + result3;; } This is the simplest, and also good test. Uros.
diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md index cce78b5..9327acf 100644 --- a/gcc/config/i386/i386.md +++ b/gcc/config/i386/i386.md @@ -176,9 +176,6 @@ ;; For CRC32 support UNSPEC_CRC32 - ;; For RDRAND support - UNSPEC_RDRAND - ;; For BMI support UNSPEC_BEXTR @@ -208,6 +205,9 @@ UNSPECV_WRFSBASE UNSPECV_WRGSBASE + ;; For RDRAND support + UNSPECV_RDRAND + ;; For RTM support UNSPECV_XBEGIN UNSPECV_XEND @@ -18399,9 +18399,9 @@ (define_insn "rdrand<mode>_1" [(set (match_operand:SWI248 0 "register_operand" "=r") - (unspec:SWI248 [(const_int 0)] UNSPEC_RDRAND)) + (unspec_volatile:SWI248 [(const_int 0)] UNSPECV_RDRAND)) (set (reg:CCC FLAGS_REG) - (unspec:CCC [(const_int 0)] UNSPEC_RDRAND))] + (unspec_volatile:CCC [(const_int 0)] UNSPECV_RDRAND))] "TARGET_RDRND" "rdrand\t%0" [(set_attr "type" "other")