[X86] Support __SSC_MARK(const int)

[xiangzhangllvm]

Summary

There are some users want to use __SSC_MARK for special use.
But They don't want to use #include xxx in they program.

So Support SSC_MARK(const int) as X86 builtin function.

[xiangzhangllvm]

Hello, @erichkeane , the patch is small and clear, and due to it changes at FE, could you help review ? Thanks a lot!

[pengfei]

clang/include/clang/Basic/BuiltinsX86.def

36

It seems using upper case letters discords current naming conversions. May be better to define __builtin_ssc_mark here and add Builder.defineMacro("__SSC_MARK", "__builtin_ssc_mark") in X86.cpp.

[xiangzhangllvm]

clang/include/clang/Basic/BuiltinsX86.def

36

Thanks, but I prefer current simple way, no touch BE code, there is no naming conversions def that built must start with __builtin and low case string.

[erichkeane]

clang/include/clang/Basic/BuiltinsX86.def

36

I very much agree with @pengfei. Builtins need to be named with __builtin and be lower case. If we want to provide a macro to be source compatible that is fine.

[xiangzhangllvm]

clang/include/clang/Basic/BuiltinsX86.def

36

There are no this rule for __builtin, for special builtin they can be directly use their original name,
For example : _mm_prefetch, _BitScanForward64, _InterlockedCompareExchange128 ...
and also ease to find the other example for other Targets.

[pengfei]

clang/include/clang/Basic/BuiltinsX86.def

36

These builtins are all Windows related, see comments "FIXME: _mm_prefetch must be a built-in because it takes a compile-time constant doesn't work in the presence of re-declaration of _mm_prefetch for windows."
These just illustrate we should follow the __builtin rule if no particular reason.
By the way, X86.cpp is a FE file. Adding a macro isn't much complicated. PPC uses a lot of macro define in this way: https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/PPC.cpp#L91

[xiangzhangllvm]

clang/include/clang/Basic/BuiltinsX86.def

36

1st I don't find such rule, even in the most basic builtin def file clang/include/clang/Basic/Builtins.def there are also lots example not follow such "rule"

2nd I don't understand what that the benefit (expect add the code in llvm) if we add a Macro which we not for other use in compiler. PPC's way is not an good example. and _mm_prefetch's comments just show why it is not normal function or Macro.

[craig.topper]

clang/lib/CodeGen/CGBuiltin.cpp

12562

Wasn't EmitScalarExpr already called for it? I think you just need Ops[0] here. You might need that to get the forced constant evaluation that was done based on ICEArguments earlier too.

[xiangzhangllvm]

clang/lib/CodeGen/CGBuiltin.cpp

12562

Good catch! It has called in line 12477, thanks very much!

[erichkeane]

clang/include/clang/Basic/BuiltinsX86.def

36

The ONLY reason we skip the __builtin rule (which YES, is a rule), is when it is to match the GCC/MSVC builtin behavior that has been under a different name for a long time.

[aaron.ballman]

clang/include/clang/Basic/BuiltinsX86.def

36

FWIW, I strongly agree that the builtin should be following typical naming conventions and use a macro if you want to spell it differently. (I happen to also be pretty unmotivated by the use case of users who don't want to #include a header file but still use the functionality from the header, so another alternative I could live with would be to not add this builtin at all.)

[erichkeane]

There are some users want to use __SSC_MARK for special use.
But They don't want to use #include xxx in they program.

Aaron said:

so another alternative I could live with would be to not add this builtin at all.)

This is a great point, the justification of "some users dont' want to use #include doesn't seem motivating to me at all. I recognize that ICC automatically includes intrin headers, but the Clang project decided years ago to not follow that on that path and implement these things in the intrinsic headers.

This is a change that doesn't belong in the frontend. I was mildly convinced as a builtin to support this (by an appropriate name), but the more I look at it, the more I realize that the code to generate the IR for this is so incredibly better done as an intrinsic entry.

So in summary: I am thoroughly convinced to reject this as a frontend change unless there is a distinct technical reason (besides "users don't want to #include").

[craig.topper]

In D108439#2962272, @erichkeane wrote:

There are some users want to use __SSC_MARK for special use.
But They don't want to use #include xxx in they program.

Aaron said:

so another alternative I could live with would be to not add this builtin at all.)

This is a great point, the justification of "some users dont' want to use #include doesn't seem motivating to me at all. I recognize that ICC automatically includes intrin headers, but the Clang project decided years ago to not follow that on that path and implement these things in the intrinsic headers.

This is a change that doesn't belong in the frontend. I was mildly convinced as a builtin to support this (by an appropriate name), but the more I look at it, the more I realize that the code to generate the IR for this is so incredibly better done as an intrinsic entry.

So in summary: I am thoroughly convinced to reject this as a frontend change unless there is a distinct technical reason (besides "users don't want to #include").

I'm not sure icc automatically includes intrinsic headers. I couldn't prove that on godbolt. It still required me to include xmmintrin.h or similar. The intrinsic functions aren't in the header, but maybe it detects that the file was included? The definition for the __m128i and similar types used by the intrinsics is in the header.

__SSC_MARK is supported by icc without a header. I'm not sure where it is documented so I don't know if it has a defined header a user could include.

[erichkeane]

In D108439#2962663, @craig.topper wrote:

In D108439#2962272, @erichkeane wrote:

There are some users want to use __SSC_MARK for special use.
But They don't want to use #include xxx in they program.

Aaron said:

so another alternative I could live with would be to not add this builtin at all.)

This is a great point, the justification of "some users dont' want to use #include doesn't seem motivating to me at all. I recognize that ICC automatically includes intrin headers, but the Clang project decided years ago to not follow that on that path and implement these things in the intrinsic headers.

This is a change that doesn't belong in the frontend. I was mildly convinced as a builtin to support this (by an appropriate name), but the more I look at it, the more I realize that the code to generate the IR for this is so incredibly better done as an intrinsic entry.

So in summary: I am thoroughly convinced to reject this as a frontend change unless there is a distinct technical reason (besides "users don't want to #include").

I'm not sure icc automatically includes intrinsic headers. I couldn't prove that on godbolt. It still required me to include xmmintrin.h or similar. The intrinsic functions aren't in the header, but maybe it detects that the file was included? The definition for the __m128i and similar types used by the intrinsics is in the header.

__SSC_MARK is supported by icc without a header. I'm not sure where it is documented so I don't know if it has a defined header a user could include.

Right, ICC has some funny-business going on with the intrinsic headers that are a little odd. We recognize/generate many of them without needing to do the #include, so it isn't really ACTUALLY automatically including the intrinsic headers, it is just automatically including a large subset of the intrinsics.

[xiangzhangllvm]

1st, I am not clear about "how ICC automatically including the intrinsic headers", But I did a try that first delete the intrinsic head file which def the SSC_MARK from ICC project, then I build SSC_MARK code with ICC, it still passed. I think if it "automatically including the intrinsic headers" it should be fail.

The ONLY reason we skip the __builtin rule (which YES, is a rule), is when it is to match the GCC/MSVC builtin behavior that has been under a different name for a long time.

2nd, I begin to agree this point, I do same check for builtins in clang/Basic/Builtins.def with GCC (include libxxx in gcc source), that matches erich's point.

[xiangzhangllvm]

implement with #include headfile at https://reviews.llvm.org/D108682
Thanks for all your careful review and discussion.

clang/include/clang/Basic/BuiltinsX86.def

 
 // Miscellaneous builtin for checking x86 cpu features.
 // TODO: Make this somewhat generic so that other backends
 // can use it?
 BUILTIN(__builtin_cpu_init, "v", "n")
 BUILTIN(__builtin_cpu_supports, "bcC*", "nc")
 BUILTIN(__builtin_cpu_is, "bcC*", "nc")
 
+TARGET_BUILTIN(__SSC_MARK, "vIi", "n", "")
+
 // Undefined Values
 //
 TARGET_BUILTIN(__builtin_ia32_undef128, "V2d", "ncV:128:", "")
 TARGET_BUILTIN(__builtin_ia32_undef256, "V4d", "ncV:256:", "")
 TARGET_BUILTIN(__builtin_ia32_undef512, "V8d", "ncV:512:", "")
 
 // FLAGS
 //

clang/lib/CodeGen/CGBuiltin.cpp

case X86::BI_mm_sfence: {
     return Builder.CreateCall(CGM.getIntrinsic(Intrinsic::x86_sse_sfence));
   }
   case X86::BI_mm_pause: {
     return Builder.CreateCall(CGM.getIntrinsic(Intrinsic::x86_sse2_pause));
   }
   case X86::BI__rdtsc: {
     return Builder.CreateCall(CGM.getIntrinsic(Intrinsic::x86_rdtsc));
   }
+  case X86::BI__SSC_MARK: {
+    llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy, {Int32Ty}, false);
+    std::string Constraints = "i,~{ebx}";
+    std::string MachineClobbers = this->getTarget().getClobbers();
+    if (!MachineClobbers.empty()) {
+      Constraints += ',';
+      Constraints += MachineClobbers;
+    }
+    llvm::InlineAsm *IA =
+        llvm::InlineAsm::get(FTy, "movl $0, %ebx; .byte 0x64, 0x67, 0x90",
+                             Constraints, /*hasSideEffects=*/true);
+    llvm::AttributeList NoReturnAttr = llvm::AttributeList::get(
+        getLLVMContext(), llvm::AttributeList::FunctionIndex,
+        llvm::Attribute::NoReturn);
+    llvm::CallInst *CI = Builder.CreateCall(IA, Ops[0]);
+    CI->setAttributes(NoReturnAttr);
+    return CI;
+  }
   case X86::BI__builtin_ia32_rdtscp: {
     Value *Call = Builder.CreateCall(CGM.getIntrinsic(Intrinsic::x86_rdtscp));
     Builder.CreateDefaultAlignedStore(Builder.CreateExtractValue(Call, 1),
                                       Ops[0]);
     return Builder.CreateExtractValue(Call, 0);
   }
   case X86::BI__builtin_ia32_lzcnt_u16:
   case X86::BI__builtin_ia32_lzcnt_u32:

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

+// RUN: %clang_cc1 -ffreestanding %s -triple=i386-unknown-unknown -emit-llvm -o - -Wall -Werror | FileCheck %s
+// RUN: %clang_cc1 -ffreestanding %s -triple x86_64-unknown-unknown -emit-llvm -o - -Wall -Werror | FileCheck %s
+
+void test_ssc_mark() {
+  // CHECK-LABEL: test_ssc_mark()
+  // CHECK: call void asm sideeffect "movl $0, %ebx; .byte 0x64, 0x67, 0x90", "i,~{ebx},~{dirflag},~{fpsr},~{flags}"(i32 7)
+  // CHECK: ret void
+  return __SSC_MARK(7);
+}