CodeGen: implement __emit intrinsic

[abdulras]

Summary

For MSVC compatibility, add the `__emit' builtin. This is used in the Windows
SDK headers, and must therefore be implemented as a builtin rather than an
intrinsic.

The `__emit' builtin provides a mechanism to emit a 32-bit opcode instruction
into the stream. The value must be a compile time constant expression. No
guarantees are made about the CPU and memory states after the execution of the
instruction.

Due to the unchecked nature of the builtin, only support this on Windows on ARM.

[t.p.northover]

Hi Reid,

[abdulras]

Hmm, seems that phabricator truncated your message Tim.

As to your concerns, I will look into creating the LLVM intrinsic for __yield as it is blessed by ACLE.

The __emit is not the prettiest of things, but that is in MSVC system headers and needs support. I am happy to make support for conditional on -fms-compatibility (Id actually prefer that, but took the path of least resistance, please be gentle!).

[abdulras]

Update to use new @llvm.arm.hint intrinsic as per the suggestion of Tim.

Depends on http://reviews.llvm.org/D3492.

[rnk]

Let's do yield as-is, but IMO emit should be a macro wrapping inline assembly in intrin.h. It has to be a macro because you'd need to use an 'I' constraint, which we don't support with always_inline.

[t.p.northover]

Yep. The yield stuff looks fine to me in that form.

Tim.

[abdulras]

@rnk, Im a bit confused here. Why do I need the I constraint for __emit? Isnt I an immediate integer valid for a data operation? The __emit takes an unsigned long and emits that long constant as an instruction.

[rnk]

In D3489#14, @abdulras wrote:

@rnk, Im a bit confused here. Why do I need the I constraint for __emit? Isnt I an immediate integer valid for a data operation? The __emit takes an unsigned long and emits that long constant as an instruction.

I was imagining this implementation in intrin.h:

#define __emit(__x) __asm __volatile (".long %0" : "i"(__x))

But it sounds like this may need to be a builtin if Windows SDK headers are using it, so this comment isn't particularly relevant.

[majnemer]

lib/Basic/Builtins.cpp

58–61 ↗

(On Diff #16934)

MSModeUnsupported uses MS_LANG and MSCompatUnsupported uses MS_MODE. That's a little confusing.

[abdulras]

lib/Basic/Builtins.cpp

58–61 ↗

(On Diff #16934)

I'm more than happy to rename this to a better name. Would you prefer MSVC_COMPAT? MSVC_MODE? MSC_COMPAT? MSC_MODE?

[majnemer]

lib/Basic/Builtins.cpp

58–61 ↗

(On Diff #16934)

Renaming MS_MODE to MS_COMPAT seems reasonable.

[abdulras]

Address review comments from majnemer

[rnk]

Do users assume that physical registers are live from one emit blob to the next? If so, I think we need to treat this as proper inline assembly and merge the blobs together like we would for asm. Otherwise, LLVM makes no guarantees that it won't insert spills between the blobs.

test/CodeGen/builtins-arm-msvc-compat-error.c

9

Why isn't sema catching this? I thought we had a builtin character code for that.

[abdulras]

Emit inserts exactly a single instruction. It provides no guarantees of the state, so I don't think that we need to worry about LLVM doing something like inserting spills.

test/CodeGen/builtins-arm-msvc-compat-error.c

9

Ill take a look and see if I can find what you are referring to. If you can point that out, that would be wonderful. If we can catch this without a fatal error, that would be significantly better.

[abdulras]

Catch the constant folding in Sema (thanks for pointing out that was possible rnk!)

[rnk]

Almost there.

include/clang/Basic/BuiltinsARM.def

88

Honestly, I think __emit is in "conforming extensions" territory, aka -fms-extensions. So I think we can drop the -fms-compatibility part.

lib/CodeGen/CGBuiltin.cpp

3171–3172

There are evil users out there using -fms-compatibility -fms-extensions on non-Windows platforms. We shouldn't assert. I think removing the assert would be fine.

3173–3174

Likewise, we shouldn't assert here. If we want to reject it, we should emit a diagnostic in sema. Since that's probably more work than just implementing ARM support, I vote for that. Just check the triple and pick between 32-bit ARM instrs and 16-bit thumb instrs (.inst vs .inst.n).

3180–3181

Maybe llvm_unreachable, since this is an internal error if sema did the right checks?

3185

What happens here if the user passes a constant larger than the instruction size, such as 0xdeadbeef for thumb? MSDN says we should truncate, but it looks like LLVM will emit an error in the backend. I guess that's OK.

test/CodeGen/builtins-arm-msvc-compat-error.c

2

This can be a -verify test now, and you can add a positive test for __emit(0xdeadbeef).

[abdulras]

include/clang/Basic/BuiltinsARM.def

88

Ugh, I *really* hate the naming. It gets me every time. Yes, you are right, this is a conforming extensions, and thus -fms-extensions not -fms-compatibility. One of these days, Ill get it right on the first try. This is not that day :-(.

lib/CodeGen/CGBuiltin.cpp

3171–3172

Ick; okay.

3173–3174

Sure; that can be useful if we were to ever support WinCE.

3180–3181

Yeah, that sounds reasonable.

3185

Thats a good point; fixed and added a test.

test/CodeGen/builtins-arm-msvc-compat-error.c

2

Ah, nice. Done.

[abdulras]

Address rnk's comments.

[rnk]

lgtm

lib/CodeGen/CGBuiltin.cpp

3180

No need to go through llvm::ConstantInt, you can just take the truncated APInt and do .getZExtValue().

[compnerd]

SVN r224438.

include/clang/Basic/Builtins.h

 #undef alloca
 
 namespace clang {
   class TargetInfo;
   class IdentifierTable;
   class ASTContext;
   class QualType;
   class LangOptions;
-  
   enum LanguageID {
     GNU_LANG = 0x1,  // builtin requires GNU mode.
     C_LANG = 0x2,    // builtin for c only.
     CXX_LANG = 0x4,  // builtin for cplusplus only.
     OBJC_LANG = 0x8, // builtin for objective-c and objective-c++
     MS_LANG = 0x10,  // builtin requires MS mode.
     ALL_LANGUAGES = C_LANG | CXX_LANG | OBJC_LANG, // builtin for all languages.
     ALL_GNU_LANGUAGES = ALL_LANGUAGES | GNU_LANG,  // builtin requires GNU mode.
     ALL_MS_LANGUAGES = ALL_LANGUAGES | MS_LANG     // builtin requires MS mode.
   };
-  
 namespace Builtin {
 enum ID {
   NotBuiltin  = 0,      // This is not a builtin function.
 #define BUILTIN(ID, TYPE, ATTRS) BI##ID,
 #include "clang/Basic/Builtins.def"
   FirstTSBuiltin
 };
 

include/clang/Basic/BuiltinsARM.def

 BUILTIN(__builtin_arm_dmb, "vUi", "nc")
 BUILTIN(__builtin_arm_dsb, "vUi", "nc")
 BUILTIN(__builtin_arm_isb, "vUi", "nc")
 
 // Prefetch
 BUILTIN(__builtin_arm_prefetch, "vvC*UiUi", "nc")
 
 // MSVC
+LANGBUILTIN(__emit, "vIUiC", "", ALL_MS_LANGUAGES)
+
 LANGBUILTIN(__yield, "v", "", ALL_MS_LANGUAGES)
 LANGBUILTIN(__wfe, "v", "", ALL_MS_LANGUAGES)
 LANGBUILTIN(__wfi, "v", "", ALL_MS_LANGUAGES)
 LANGBUILTIN(__sev, "v", "", ALL_MS_LANGUAGES)
 LANGBUILTIN(__sevl, "v", "", ALL_MS_LANGUAGES)
 
 LANGBUILTIN(__dmb, "vUi", "nc", ALL_MS_LANGUAGES)
 LANGBUILTIN(__dsb, "vUi", "nc", ALL_MS_LANGUAGES)

lib/CodeGen/CGBuiltin.cpp

 #include "CGObjCRuntime.h"
 #include "CodeGenModule.h"
 #include "TargetInfo.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/Basic/TargetBuiltins.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/IR/DataLayout.h"
+#include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/Intrinsics.h"
 
 using namespace clang;
 using namespace CodeGen;
 using namespace llvm;
 
 /// getBuiltinLibFunction - Given a builtin id for a function like
 /// "__builtin_fabsf", return a Function* for "fabsf".

   }
 }
 
 Value *CodeGenFunction::EmitARMBuiltinExpr(unsigned BuiltinID,
                                            const CallExpr *E) {
   if (auto Hint = GetValueForARMHint(BuiltinID))
     return Hint;
 
+  if (BuiltinID == ARM::BI__emit) {
+    bool IsThumb = getTarget().getTriple().getArch() == llvm::Triple::thumb;
+    llvm::FunctionType *FTy =
+        llvm::FunctionType::get(VoidTy, /*Variadic=*/false);
+
+    APSInt Value;
+    if (!E->getArg(0)->EvaluateAsInt(Value, CGM.getContext()))
+      llvm_unreachable("Sema will ensure that the parameter is constant");
+
+    uint64_t ZExtValue =
+        Builder.getInt(Value.zextOrTrunc(IsThumb ? 16 : 32))->getZExtValue();
+
+    llvm::InlineAsm *Emit =
+        IsThumb ? InlineAsm::get(FTy, ".inst.n 0x" + utohexstr(ZExtValue), "",
+                                 /*SideEffects=*/true)
+                : InlineAsm::get(FTy, ".inst 0x" + utohexstr(ZExtValue), "",
+                                 /*SideEffects=*/true);
+
+    return Builder.CreateCall(Emit);
+  }
+
   if (BuiltinID == ARM::BI__builtin_arm_dbg) {
     Value *Option = EmitScalarExpr(E->getArg(0));
     return Builder.CreateCall(CGM.getIntrinsic(Intrinsic::arm_dbg), Option);
   }
 
   if (BuiltinID == ARM::BI__builtin_arm_prefetch) {
     Value *Address = EmitScalarExpr(E->getArg(0));
     Value *RW      = EmitScalarExpr(E->getArg(1));

test/CodeGen/builtins-arm-msvc-compat-error.c

+// RUN: %clang_cc1 -triple thumbv7-windows -fms-compatibility -verify %s
+
+void emit_error(unsigned int opcode) {
+  __emit(opcode); // expected-error {{argument to '__emit' must be a constant integer}}
+}
+

test/CodeGen/builtins-arm-msvc-compat-only.c

+// RUN: %clang_cc1 -triple thumbv7-windows -fms-compatibility -emit-llvm -o - %s \
+// RUN:     | FileCheck %s -check-prefix CHECK-MSVC
+// RUN: %clang_cc1 -triple armv7-eabi -emit-llvm %s -o /dev/null 2>&1 \
+// RUN:     | FileCheck %s -check-prefix CHECK-EABI
+// REQUIRES: arm-registered-target
+
+void emit() {
+  __emit(0xdefe);
+}
+
+// CHECK-MSVC: call void asm sideeffect ".inst.n 0xDEFE", ""()
+// CHECK-EABI: warning: implicit declaration of function '__emit' is invalid in C99
+
+void emit_truncated() {
+  __emit(0x11110000); // movs r0, r0
+}
+
+// CHECK-MSVC: call void asm sideeffect ".inst.n 0x0", ""()
+