Implement __cpuid and __cpuidex as Clang builtins

[ayzhao]

Summary

https://reviews.llvm.org/D23944 implemented the #pragma intrinsic from
MSVC. This causes the statement #pragma intrinsic(cpuid) to fail [0]
on Clang because cpuid is currently implemented in intrin.h instead
of a Clang builtin. Reimplementing cpuid (as well as it's releated
function, cpuidex) should resolve this.

[0]: https://crbug.com/1279344

[craig.topper]

Why does this need to be a builtin? What's wrong with the current implementation in the header file?

[ayzhao]

The purpose of this patch is to make #pragma intrinsic work with __cpuid. https://reviews.llvm.org/D98438 implemented support for #pragma intrinsic. This resulted in a bug in Chrome (https://crbug.com/1279344) where #pragma intrinsic(__cpuid) works in MSVC but doesn't work on Clang since Clang implements __cpuid in the header file. Clang's implementation of #pragma intrinsic only recognizes Clang builtins, so this patch should fix that.

As an aside, this patch is a _very_ early WIP that doesn't work and as such is not meant for review.

[ayzhao]

Implement cpuid and cpuidex as Clang builtins

https://reviews.llvm.org/D23944 implemented the #pragma intrinsic from
MSVC. This causes the statement #pragma intrinsic(cpuid) to fail [0]
on Clang because cpuid is currently implemented in intrin.h instead
of a Clang builtin. Reimplementing cpuid (as well as it's releated
function, cpuidex) should resolve this.

[0]: https://crbug.com/1279344

[hans]

clang/lib/CodeGen/CGBuiltin.cpp

14917

nit: perhaps the "Arg" suffix is unnecessary (also for the next variable)

14923

Since Asm isn't going to change here, maybe having a variable isn't worth it; you could just pass "cpuid" directly to InlineAsm::get().

If you want to keep the variable for clarity, I think StringRef would be the better type. SmallString is normally used as a small buffer on the stack when building a string gradually. And since we're not building much, I'd also drop the comment above.

14924

I know this is based on EmitX86BitTestIntrinsic(), but instead of using std::strings here, I think this would be an example of a good use of SmallString to build Constraints.

However, I'm not sure we need to add getTarget().getClobbers() here at all. From checking the Intel manual, I don't believe "cpuid" clobbers anything besides the output registers. That's also how the instruction is modeled in llvm: https://github.com/llvm/llvm-project/blob/llvmorg-13.0.1/llvm/lib/Target/X86/X86InstrSystem.td#L452

For EmitX86BitTestIntrinsic(), Craig added the extra clobbers in D88121. Craig, can you comment on why we seem to set these clobbers everywhere?

14937

I don't think hasSideEffects needs to be true for cpuid.
Maybe Craig can confirm?

14942

(just using 'int' would be more common llvm style)

14949

(technically we already emitted the intrinsic, the value is really to signal that we successfully did so)

clang/lib/Headers/intrin.h

538

I wonder if we could remove __cpuid_count too. It seems it was added in D101338 just for the __cpuid(ex) implementations.

clang/test/CodeGen/ms-intrinsics-cpuid.c

11–12

It would be good to test that #pragma intrinsic doesn't generate a warning for __cpuid anymore. Just inserting it here and adding -Werror to the run lines would work.

17–18

Since the test is very exact, maybe it would make sense to take it all the way by giving symbolic names to the function arguments and check that they get passed correctly to the inline asm, and used to store the results.

[ayzhao]

Addressed some code review comments

[ayzhao]

clang/lib/CodeGen/CGBuiltin.cpp

14923

Ended up just passing "cpuid" directly

14924

Changed this to use SmallString.

Leaving this comment open until we figure out whether or not we need the clobbers.

clang/lib/Headers/intrin.h

538

I'm always in favor of removing dead code. I do have concerns that because of Hyrum's Law, we'll break users who call __cpuid_count even though it's an undocumented and unsupported API. Is this acceptable?

clang/test/CodeGen/ms-intrinsics-cpuid.c

17–18

This doesn't seem feasible to implement.

The reason is that in the unoptimized output, Clang emits a load for the same value of cpu_info 4 times, once for each getelementptr instruction. When it comes time to call asm, Clang can pass the output of any of the 4 load instructions to the asm, and explicitly specifying a single load instruction output as the parameter would make the test flaky.

One option would be to [[ https://godbolt.org/z/qY9ja1s11 | pass -O1 to Clang ]], which causes Clang to reuse the input registers and removes unnecessary loads and stores, so we can just use the input registers in the test's expected output. WDYT?

[craig.topper]

clang/lib/CodeGen/CGBuiltin.cpp

14924

I don't know the full history here. My changed in D88121 was to get the list from the getClobbers() instead of hardcoding "~{flags},~{fpsr}" into the string. And to add the "~{dirflag}". At the time I assumed dirflag missing was an oversight. getClobbers is what gets used for all user created inline assembly.

I think we don't need the clobbers here.

[ayzhao]

Removed CPU clobbers

[ayzhao]

clang/lib/CodeGen/CGBuiltin.cpp

14924

Clobbers have been removed.

[hans]

clang/lib/CodeGen/CGBuiltin.cpp

14931

I think hasSideEffects should be false. I believe that will allow LLVM to optimize away the inline asm if the results are not used.

Craig, does that sound right to you?

14938

Oh, do we get a warning/error if this just uses i directly? In that case maybe using "unsigned" for the loop variable is better.

clang/lib/Headers/intrin.h

538

I think D101338 is recent enough that it will be okay.

clang/test/CodeGen/ms-intrinsics-cpuid.c

17–18

Oh right, yes it would get messy. Let's skip it then.

[ayzhao]

Remove __cpuid_count and use unsigned in the for loop variable

[craig.topper]

clang/lib/CodeGen/CGBuiltin.cpp

14931

I agree. It wasn't a volatile asm in the header file so we shouldn't need side effects.

14946

I'm a little surprised we're able to return a dummy i1 value as the result of a void builtin. Is this done anywhere else?

clang/lib/Headers/intrin.h

545

What are we doing about this xchg that's needed to generate correct code?

[craig.topper]

clang/lib/CodeGen/CGBuiltin.cpp

14944

You should probably just return the last store you created. See BI__builtin_neon_vzipq_v for example.

[craig.topper]

clang/lib/CodeGen/CGBuiltin.cpp

14927

you shouldn't need llvm:: here. There's a using llvm:StringRef in effect on this file.

You also don't needs static or constexpr. StringRef's don't own anything and are cheap to create.

[ayzhao]

Set hasSideEffects=false, other fixes

clang/lib/CodeGen/CGBuiltin.cpp

14931

hasSideEffects is now false

14946

Ended up just returning the last store

[ayzhao]

Implement xchgq workaround for x86_64

[ayzhao]

Update xchg implementation so that it uses the one it replaced in intrin.h instead of the one from cpuid.h

It's not identical to the one in intrin.h as InlineAsm isn't exactly tolerant of everything that's legal in an asm statement, so I ran the implementation from intrin.h through clang and copy/pasted it from the IR.

[ayzhao]

clang/lib/Headers/intrin.h

545

The xchg instructions are now in the output for x86_64

I originally thought that the =b constraint was sufficient to preserve rbx, so I used the same implementation for both x86 and x86_64. However, I discovered https://bugs.llvm.org/show_bug.cgi?id=20311 where it turns out that in certain cases, =b is not sufficient.

[hans]

clang/lib/CodeGen/CGBuiltin.cpp

14933

The `${...|...}` syntax is used to support multiple inline asm dialects (At&t or Intel), see https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Multiple-assembler-dialects-in-asm-templates

The syntax was used in intrin.h, because that file could be built using either asm dialect depending on what options the compiler is run with. But now when we're calling InlineAsm::get directly, I think it's not needed and we should just use the At&t style (because InlineAsm::get's asmDialect param defaults to that).

14945

(nit: llvm code typically just writes it 'unsigned' without the 'int' part)

[ayzhao]

Use AT&T syntax for assembly, unsigned int -> unsigned

[hans]

lgtm

Since Craig reviewed, let's wait for him too before landing.

[craig.topper]

LGTM to me other than those two minor comments.

clang/lib/CodeGen/CGBuiltin.cpp

14946

Why do we need curly braces around i?

14947

I think you can use Builder.CreateConstInBoundsGEP1_32(Int32Ty, BasePtr, i) to remove the need to call to ConstantInt::get.

[ayzhao]

Remove curly braces and call to ConstantInt::get

[craig.topper]

LGTM

clang/include/clang/Basic/BuiltinsX86.def

 // MSVC
 TARGET_HEADER_BUILTIN(_BitScanForward, "UcUNi*UNi", "nh", "intrin.h", ALL_MS_LANGUAGES, "")
 TARGET_HEADER_BUILTIN(_BitScanReverse, "UcUNi*UNi", "nh", "intrin.h", ALL_MS_LANGUAGES, "")
 
 TARGET_HEADER_BUILTIN(_ReadWriteBarrier, "v", "nh", "intrin.h", ALL_MS_LANGUAGES, "")
 TARGET_HEADER_BUILTIN(_ReadBarrier,      "v", "nh", "intrin.h", ALL_MS_LANGUAGES, "")
 TARGET_HEADER_BUILTIN(_WriteBarrier,     "v", "nh", "intrin.h", ALL_MS_LANGUAGES, "")
 
+TARGET_HEADER_BUILTIN(__cpuid,   "vi*i",  "nh", "intrin.h", ALL_MS_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(__cpuidex, "vi*ii", "nh", "intrin.h", ALL_MS_LANGUAGES, "")
+
 TARGET_HEADER_BUILTIN(__emul,  "LLiii",    "nch", "intrin.h", ALL_MS_LANGUAGES, "")
 TARGET_HEADER_BUILTIN(__emulu, "ULLiUiUi", "nch", "intrin.h", ALL_MS_LANGUAGES, "")
 
 TARGET_HEADER_BUILTIN(_AddressOfReturnAddress, "v*", "nh", "intrin.h", ALL_MS_LANGUAGES, "")
 
 TARGET_HEADER_BUILTIN(__stosb, "vUc*Ucz", "nh", "intrin.h", ALL_MS_LANGUAGES, "")
 TARGET_HEADER_BUILTIN(__int2c, "v",       "nhr", "intrin.h", ALL_MS_LANGUAGES, "")
 TARGET_HEADER_BUILTIN(__ud2,   "v",       "nhr", "intrin.h", ALL_MS_LANGUAGES, "")

clang/lib/CodeGen/CGBuiltin.cpp

case X86::BI__builtin_ia32_cvtneps2bf16_512_mask: {
     case X86::BI__builtin_ia32_cvtneps2bf16_512_mask:
       IID = Intrinsic::x86_avx512bf16_cvtneps2bf16_512;
       break;
     }
     Value *Res = Builder.CreateCall(CGM.getIntrinsic(IID), Ops[0]);
     return EmitX86Select(*this, Ops[2], Res, Ops[1]);
   }
 
+  case X86::BI__cpuid:
+  case X86::BI__cpuidex: {
+    Value *FuncId = EmitScalarExpr(E->getArg(1));
+    Value *SubFuncId = BuiltinID == X86::BI__cpuidex
+                           ? EmitScalarExpr(E->getArg(2))
+                           : llvm::ConstantInt::get(Int32Ty, 0);
+
+    static constexpr llvm::StringRef Constraints =
+        "={ax},={bx},={cx},={dx},{ax},{cx}";
+
+    llvm::StructType *CpuidRetTy =
+        llvm::StructType::get(Int32Ty, Int32Ty, Int32Ty, Int32Ty);
+    llvm::FunctionType *FTy =
+        llvm::FunctionType::get(CpuidRetTy, {Int32Ty, Int32Ty}, false);
+
+    llvm::InlineAsm *IA = llvm::InlineAsm::get(FTy, "cpuid", Constraints,
+                                               /*hasSideEffects=*/true);
+    llvm::Value *IACall = Builder.CreateCall(IA, {FuncId, SubFuncId});
+
+    Value *BasePtr = EmitScalarExpr(E->getArg(0));
+
+    for (int i = 0; i < 4; i++) {
+      Value *Extracted =
+          Builder.CreateExtractValue(IACall, {static_cast<unsigned int>(i)});
+      Value *StorePtr = Builder.CreateInBoundsGEP(
+          Int32Ty, BasePtr, llvm::ConstantInt::get(Int32Ty, i));
+      Builder.CreateAlignedStore(Extracted, StorePtr, getIntAlign());
+    }
+
+    // Return a dummy value to signal that we have successfully emitted the
+    // intrinsic.
+    return Builder.getTrue();
+  }
+
   case X86::BI__emul:
   case X86::BI__emulu: {
     llvm::Type *Int64Ty = llvm::IntegerType::get(getLLVMContext(), 64);
     bool isSigned = (BuiltinID == X86::BI__emul);
     Value *LHS = Builder.CreateIntCast(Ops[0], Int64Ty, isSigned);
     Value *RHS = Builder.CreateIntCast(Ops[1], Int64Ty, isSigned);
     return Builder.CreateMul(LHS, RHS, "", !isSigned, isSigned);
   }
   case X86::BI__mulh:

clang/lib/Headers/intrin.h

 }
 #endif
 
 /*----------------------------------------------------------------------------*\
 |* Misc
 \*----------------------------------------------------------------------------*/
 #if defined(__i386__) || defined(__x86_64__)
 #if defined(__i386__)
 #define __cpuid_count(__leaf, __count, __eax, __ebx, __ecx, __edx)             \
   __asm("cpuid"                                                                \
         : "=a"(__eax), "=b"(__ebx), "=c"(__ecx), "=d"(__edx)                   \
         : "0"(__leaf), "2"(__count))
 #else
 /* x86-64 uses %rbx as the base register, so preserve it. */
 #define __cpuid_count(__leaf, __count, __eax, __ebx, __ecx, __edx)             \
   __asm("xchg{q} {%%rbx, %q1|%q1, rbx}\n"                                      \
         "cpuid\n"                                                              \
         "xchg{q} {%%rbx, %q1|%q1, rbx}"                                        \
         : "=a"(__eax), "=r"(__ebx), "=c"(__ecx), "=d"(__edx)                   \
         : "0"(__leaf), "2"(__count))
 #endif
-static __inline__ void __DEFAULT_FN_ATTRS __cpuid(int __info[4], int __level) {
-  __cpuid_count(__level, 0, __info[0], __info[1], __info[2], __info[3]);
-}
-static __inline__ void __DEFAULT_FN_ATTRS __cpuidex(int __info[4], int __level,
-                                                    int __ecx) {
-  __cpuid_count(__level, __ecx, __info[0], __info[1], __info[2], __info[3]);
-}
 static __inline__ void __DEFAULT_FN_ATTRS __halt(void) {
   __asm__ volatile("hlt");
 }
 #endif
 
 #if defined(__i386__) || defined(__x86_64__) || defined(__aarch64__)
 static __inline__ void __DEFAULT_FN_ATTRS __nop(void) {
   __asm__ volatile("nop");

clang/test/CodeGen/ms-intrinsics-cpuid.c

 // RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility -fms-compatibility-version=17.00 \
-// RUN:         -triple i686-windows-msvc -emit-llvm %s -o - | FileCheck %s --check-prefix=X86
+// RUN:         -Werror -triple i686-windows-msvc -emit-llvm %s -o - | FileCheck %s --check-prefix=X86
 // RUN: %clang_cc1 -ffreestanding -fms-extensions -fms-compatibility -fms-compatibility-version=17.00 \
-// RUN:         -triple x86_64-windows-msvc -emit-llvm %s -o - | FileCheck %s --check-prefix=X64
+// RUN:         -Werror -triple x86_64-windows-msvc -emit-llvm %s -o - | FileCheck %s --check-prefix=X64
 
 // intrin.h needs size_t, but -ffreestanding prevents us from getting it from
 // stddef.h.  Work around it with this typedef.
 typedef __SIZE_TYPE__ size_t;
 
 #include <intrin.h>
 
-void test__cpuid(int *info, int level) {
-  __cpuid(info, level);
+#pragma intrinsic(__cpuid)
+
+void test__cpuid(int cpuInfo[4], int function_id) {
+  __cpuid(cpuInfo, function_id);
 }
 // X86-LABEL: define {{.*}} @test__cpuid(i32* noundef %{{.*}}, i32 noundef %{{.*}})
-// X86: call { i32, i32, i32, i32 } asm "cpuid",
-// X86-SAME:   "={ax},={bx},={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"
-// X86-SAME:   (i32 %{{.*}}, i32 0)
+// X86-DAG: [[ASMRESULTS:%[0-9]+]] = call { i32, i32, i32, i32 } asm sideeffect "cpuid", "={ax},={bx},={cx},={dx},{ax},{cx}
+// X86-DAG: [[ADDRPTR0:%[0-9]+]] = getelementptr inbounds i32, i32* %{{.*}}, i32 0
+// X86-DAG: [[ADDRPTR1:%[0-9]+]] = getelementptr inbounds i32, i32* %{{.*}}, i32 1
+// X86-DAG: [[ADDRPTR2:%[0-9]+]] = getelementptr inbounds i32, i32* %{{.*}}, i32 2
+// X86-DAG: [[ADDRPTR3:%[0-9]+]] = getelementptr inbounds i32, i32* %{{.*}}, i32 3
+// X86-DAG: [[RESULT0:%[0-9]+]] = extractvalue { i32, i32, i32, i32 } [[ASMRESULTS]], 0
+// X86-DAG: [[RESULT1:%[0-9]+]] = extractvalue { i32, i32, i32, i32 } [[ASMRESULTS]], 1
+// X86-DAG: [[RESULT2:%[0-9]+]] = extractvalue { i32, i32, i32, i32 } [[ASMRESULTS]], 2
+// X86-DAG: [[RESULT3:%[0-9]+]] = extractvalue { i32, i32, i32, i32 } [[ASMRESULTS]], 3
+// X86-DAG: store i32 [[RESULT0]], i32* [[ADDRPTR0]], align 4
+// X86-DAG: store i32 [[RESULT1]], i32* [[ADDRPTR1]], align 4
+// X86-DAG: store i32 [[RESULT2]], i32* [[ADDRPTR2]], align 4
+// X86-DAG: store i32 [[RESULT3]], i32* [[ADDRPTR3]], align 4
 
 // X64-LABEL: define {{.*}} @test__cpuid(i32* noundef %{{.*}}, i32 noundef %{{.*}})
-// X64: call { i32, i32, i32, i32 } asm "xchg$(q$) $(%rbx{{.*}}$){{.*}}cpuid{{.*}}xchg$(q$) $(%rbx{{.*}}$)",
-// X64-SAME:   "={ax},=r,={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"
-// X64-SAME:   (i32 %{{.*}}, i32 0)
+// X64-DAG: [[ASMRESULTS:%[0-9]+]] = call { i32, i32, i32, i32 } asm sideeffect "cpuid", "={ax},={bx},={cx},={dx},{ax},{cx}
+// X64-DAG: [[ADDRPTR0:%[0-9]+]] = getelementptr inbounds i32, i32* %{{.*}}, i32 0
+// X64-DAG: [[ADDRPTR1:%[0-9]+]] = getelementptr inbounds i32, i32* %{{.*}}, i32 1
+// X64-DAG: [[ADDRPTR2:%[0-9]+]] = getelementptr inbounds i32, i32* %{{.*}}, i32 2
+// X64-DAG: [[ADDRPTR3:%[0-9]+]] = getelementptr inbounds i32, i32* %{{.*}}, i32 3
+// X64-DAG: [[RESULT0:%[0-9]+]] = extractvalue { i32, i32, i32, i32 } [[ASMRESULTS]], 0
+// X64-DAG: [[RESULT1:%[0-9]+]] = extractvalue { i32, i32, i32, i32 } [[ASMRESULTS]], 1
+// X64-DAG: [[RESULT2:%[0-9]+]] = extractvalue { i32, i32, i32, i32 } [[ASMRESULTS]], 2
+// X64-DAG: [[RESULT3:%[0-9]+]] = extractvalue { i32, i32, i32, i32 } [[ASMRESULTS]], 3
+// X64-DAG: store i32 [[RESULT0]], i32* [[ADDRPTR0]], align 4
+// X64-DAG: store i32 [[RESULT1]], i32* [[ADDRPTR1]], align 4
+// X64-DAG: store i32 [[RESULT2]], i32* [[ADDRPTR2]], align 4
+// X64-DAG: store i32 [[RESULT3]], i32* [[ADDRPTR3]], align 4
+
+#pragma intrinsic(__cpuidex)
+
+void test__cpuidex(int cpuInfo[4], int function_id, int subfunction_id) {
+  __cpuidex(cpuInfo, function_id, subfunction_id);
+}
+// X86-LABEL: define {{.*}} @test__cpuidex(i32* noundef %{{.*}}, i32 noundef %{{.*}}, i32 noundef %{{.*}})
+// X86-DAG: [[ASMRESULTS:%[0-9]+]] = call { i32, i32, i32, i32 } asm sideeffect "cpuid", "={ax},={bx},={cx},={dx},{ax},{cx}
+// X86-DAG: [[ADDRPTR0:%[0-9]+]] = getelementptr inbounds i32, i32* %{{.*}}, i32 0
+// X86-DAG: [[ADDRPTR1:%[0-9]+]] = getelementptr inbounds i32, i32* %{{.*}}, i32 1
+// X86-DAG: [[ADDRPTR2:%[0-9]+]] = getelementptr inbounds i32, i32* %{{.*}}, i32 2
+// X86-DAG: [[ADDRPTR3:%[0-9]+]] = getelementptr inbounds i32, i32* %{{.*}}, i32 3
+// X86-DAG: [[RESULT0:%[0-9]+]] = extractvalue { i32, i32, i32, i32 } [[ASMRESULTS]], 0
+// X86-DAG: [[RESULT1:%[0-9]+]] = extractvalue { i32, i32, i32, i32 } [[ASMRESULTS]], 1
+// X86-DAG: [[RESULT2:%[0-9]+]] = extractvalue { i32, i32, i32, i32 } [[ASMRESULTS]], 2
+// X86-DAG: [[RESULT3:%[0-9]+]] = extractvalue { i32, i32, i32, i32 } [[ASMRESULTS]], 3
+// X86-DAG: store i32 [[RESULT0]], i32* [[ADDRPTR0]], align 4
+// X86-DAG: store i32 [[RESULT1]], i32* [[ADDRPTR1]], align 4
+// X86-DAG: store i32 [[RESULT2]], i32* [[ADDRPTR2]], align 4
+// X86-DAG: store i32 [[RESULT3]], i32* [[ADDRPTR3]], align 4
+
+// X64-LABEL: define {{.*}} @test__cpuidex(i32* noundef %{{.*}}, i32 noundef %{{.*}}, i32 noundef %{{.*}})
+// X64-DAG: [[ASMRESULTS:%[0-9]+]] = call { i32, i32, i32, i32 } asm sideeffect "cpuid", "={ax},={bx},={cx},={dx},{ax},{cx}
+// X64-DAG: [[ADDRPTR0:%[0-9]+]] = getelementptr inbounds i32, i32* %{{.*}}, i32 0
+// X64-DAG: [[ADDRPTR1:%[0-9]+]] = getelementptr inbounds i32, i32* %{{.*}}, i32 1
+// X64-DAG: [[ADDRPTR2:%[0-9]+]] = getelementptr inbounds i32, i32* %{{.*}}, i32 2
+// X64-DAG: [[ADDRPTR3:%[0-9]+]] = getelementptr inbounds i32, i32* %{{.*}}, i32 3
+// X64-DAG: [[RESULT0:%[0-9]+]] = extractvalue { i32, i32, i32, i32 } [[ASMRESULTS]], 0
+// X64-DAG: [[RESULT1:%[0-9]+]] = extractvalue { i32, i32, i32, i32 } [[ASMRESULTS]], 1
+// X64-DAG: [[RESULT2:%[0-9]+]] = extractvalue { i32, i32, i32, i32 } [[ASMRESULTS]], 2
+// X64-DAG: [[RESULT3:%[0-9]+]] = extractvalue { i32, i32, i32, i32 } [[ASMRESULTS]], 3
+// X64-DAG: store i32 [[RESULT0]], i32* [[ADDRPTR0]], align 4
+// X64-DAG: store i32 [[RESULT1]], i32* [[ADDRPTR1]], align 4
+// X64-DAG: store i32 [[RESULT2]], i32* [[ADDRPTR2]], align 4
+// X64-DAG: store i32 [[RESULT3]], i32* [[ADDRPTR3]], align 4