[X86] [BugFix] Add 64 bit implement for __SSC_MARK

[xiangzhangllvm]

Summary

When in 64-bit target, the 32-bit operands generate a 32-bit result,
zero-extended to a 64-bit result in the destination general-purpose.

It means "mov x %ebx" will clobber the higher 32 bits of rbx, so we
should preserve the 64-bit register rbx in __SSC_MARK.

[pengfei]

LGTM.

clang/lib/Headers/x86gprintrin.h

28

I seems we usually use #ifdef __i386__ or #if defined(__i386__)

[jsji]

Thanks for fixing!

clang/lib/Headers/x86gprintrin.h

50

Duplicate inline assembly block will be harder to maintain.

How about something like:

#if defined(__i386__)
#define FULLBX "ebx"
#define TMPGPR "eax"
#else
#define FULLBX "rbx"
#define TMPGPR "rax"
#endif

#define MOVEGPR(r1,r2) "mov {%%" r1 ", %%" r2 "|" r2 ", " r1 "}; "

#define SAVE_GPRBX MOVEGPR(FULLBX, TMPGPR)
#define RESTORE_GPRBX MOVEGPR(TMPGPR, FULLBX)

#define __SSC_MARK(Tag) \
  __asm__ __volatile__( SAVE_GPRBX \
                       "mov {%0, %%ebx|ebx, %0}; "                          \
                       ".byte 0x64, 0x67, 0x90; "                              \
                        RESTORE_GPRBX \
                       ::"i"(Tag)            \
                       :  TMPGPR );

clang/test/CodeGen/X86/x86-ssc-mark.c

19–28

nit: it might be better to use a magic number other than 0, to avoid bugs of 0 vs %0 in inline assembly.

[xiangzhangllvm]

clang/lib/Headers/x86gprintrin.h

28

Yes that is more standard.

50

Thanks a lot for your careful reviewing!
First very sorry for late respond! I take days leave.
This way is good for Duplication.
Just a small flaw: do you think it is a little complex (from readability) for 2 small inline asm : )

[xiangzhangllvm]

clang/test/CodeGen/X86/x86-ssc-mark.c

19–28

Sorry, I am a little puzzle here, the 0 is parameter's value ($0 in asm), and %0 is the order for this parameter. Does you mean the %0 maybe ignore?

[jsji]

clang/lib/Headers/x86gprintrin.h

50

I think it is better for both readability and maintainability -- even the new comer will be able to distinguish the part for save/restore and the part of inline asm for ssc_mark.

clang/test/CodeGen/X86/x86-ssc-mark.c

19–28

I meant if there is bug in inline asm, someone write 0 instead of %0, then the testcase won't be able to expose it.

[xiangzhangllvm]

clang/test/CodeGen/X86/x86-ssc-mark.c

19–28

I got it, that make sense, thank you!

[xiangzhangllvm]

Address JinSong and Pengfei's comments

[jsji]

LGTM, thanks for fixing!

[craig.topper]

clang/lib/Headers/x86gprintrin.h

36

All names in header files must start with double underscores. And they should be undefined when you’re done with them.

[xiangzhangllvm]

clang/lib/Headers/x86gprintrin.h

36

Hi Craig, thanks for your remind. I'll add "double underscores"
But If I undef these macros, the SSC_MARK will not work.
I think the code use SSC_MARK (not in this head file, but in the test case) will first meet "#undef"

28 #if defined(__i386__)
29 #define __FULLBX "ebx"
30 #define __TMPGPR "eax"
31 #else
32 // When in 64-bit target, the 32-bit operands generate a 32-bit result,
33 // zero-extended to a 64-bit result in the destination general-purpose,
34 // It means "mov x %ebx" will clobber the higher 32 bits of rbx, so we
35 // should preserve the 64-bit register rbx.
36 #define __FULLBX "rbx"
37 #define __TMPGPR "rax"
38 #endif
39
40 #define __MOVEGPR(__r1, __r2) "mov {%%"__r1 ", %%"__r2 "|"__r2 ", "__r1"};"
41
42 #define __SAVE_GPRBX __MOVEGPR(__FULLBX, __TMPGPR)
43 #define __RESTORE_GPRBX __MOVEGPR(__TMPGPR, __FULLBX)
44
45 #define __TMPGPR "rax"
46 #define __SSC_MARK(__Tag)                                                      \
47   __asm__ __volatile__( __SAVE_GPRBX                                           \
48                        "mov {%0, %%ebx|ebx, %0}; "                             \
49                        ".byte 0x64, 0x67, 0x90; "                              \
50                         __RESTORE_GPRBX                                        \
51                        ::"i"(__Tag)                                            \
52                        :  __TMPGPR );
53 
54 #undef __FULLBX
55 #undef __TMPGPR
56 #undef __MOVEGPR
57 #undef __SAVE_GPRBX
58 #undef __RESTORE_GPRBX
59

Take this test case for example, So result will be
(clang -target x86_64-unknown-unknown -ffreestanding clang/test/CodeGen/X86/x86-ssc-mark.c -E -o t.i)

void ssc_mark(void) {
 # 28 "clang/test/CodeGen/X86/x86-ssc-mark.c"
   __asm__ __volatile__( __SAVE_GPRBX "mov {%0, %%ebx|ebx, %0}; " ".byte 0x64, 0x67, 0x90; " __RESTORE_GPRBX ::"i"(0x9) : __TMPGPR );;
 }

[xiangzhangllvm]

So let me first add "double underscores", many thanks!

[xiangzhangllvm]

Add "double underscores" for all name used in __SSC_MARK

[craig.topper]

LGTM

clang/lib/Headers/x86gprintrin.h

 #include <uintrintrin.h>
 #endif
 
 #if !(defined(_MSC_VER) || defined(__SCE__)) || __has_feature(modules) ||      \
     defined(__CRC32__)
 #include <crc32intrin.h>
 #endif
 
-#define __SSC_MARK(Tag)                                                        \
-  __asm__ __volatile__("mov {%%ebx, %%eax|eax, ebx}; "                      \
-                       "mov {%0, %%ebx|ebx, %0}; "                          \
+#if defined(__i386__)
+#define __FULLBX "ebx"
+#define __TMPGPR "eax"
+#else
+// When in 64-bit target, the 32-bit operands generate a 32-bit result,
+// zero-extended to a 64-bit result in the destination general-purpose,
+// It means "mov x %ebx" will clobber the higher 32 bits of rbx, so we
+// should preserve the 64-bit register rbx.
+#define __FULLBX "rbx"
+#define __TMPGPR "rax"
+#endif
+
+#define __MOVEGPR(__r1, __r2) "mov {%%"__r1 ", %%"__r2 "|"__r2 ", "__r1"};"
+
+#define __SAVE_GPRBX __MOVEGPR(__FULLBX, __TMPGPR)
+#define __RESTORE_GPRBX __MOVEGPR(__TMPGPR, __FULLBX)
+
+#define __SSC_MARK(__Tag)                                                      \
+  __asm__ __volatile__( __SAVE_GPRBX                                           \
+                       "mov {%0, %%ebx|ebx, %0}; "                             \
                        ".byte 0x64, 0x67, 0x90; "                              \
-                       "mov {%%eax, %%ebx|ebx, eax};" ::"i"(Tag)            \
-                       : "%eax");
+                        __RESTORE_GPRBX                                        \
+                       ::"i"(__Tag)                                            \
+                       :  __TMPGPR );
 
 #endif /* __X86GPRINTRIN_H */

clang/test/CodeGen/X86/x86-ssc-mark.c

 // REQUIRES: x86-registered-target
-// RUN: %clang_cc1 %s -triple=x86_64-unknown-unknown -S -ffreestanding -o - | FileCheck %s
-// RUN: %clang_cc1 %s -triple=i386-unknown-unknown -S -ffreestanding -o - | FileCheck %s
+// RUN: %clang_cc1 %s -triple=i386-unknown-unknown -S -ffreestanding -o - | FileCheck %s --check-prefix=X86
+// RUN: %clang_cc1 %s -triple=x86_64-unknown-unknown -S -ffreestanding -o - | FileCheck %s --check-prefix=X64
 
 #include <immintrin.h>
 
 // The ebx may be use for base pointer, we need to restore it in time.
 void ssc_mark(void) {
-// CHECK-LABEL: ssc_mark
-// CHECK: #APP
-// CHECK: movl    %ebx, %eax
-// CHECK: movl    $0, %ebx
-// CHECK: .byte   100
-// CHECK: .byte   103
-// CHECK: .byte   144
-// CHECK: movl    %eax, %ebx
-// CHECK: #NO_APP
+// X86-LABEL: ssc_mark
+// X86: #APP
+// X86: movl    %ebx, %eax
+// X86: movl    $9, %ebx
+// X86: .byte   100
+// X86: .byte   103
+// X86: .byte   144
+// X86: movl    %eax, %ebx
+// X86: #NO_APP
 
-  __SSC_MARK(0x0);
+// X64-LABEL: ssc_mark
+// X64: #APP
+// X64: movq    %rbx, %rax
+// X64: movl    $9, %ebx
+// X64: .byte   100
+// X64: .byte   103
+// X64: .byte   144
+// X64: movq    %rax, %rbx
+// X64: #NO_APP
+  __SSC_MARK(0x9);
 }