Page MenuHomePhabricator

CodeGen: implement __emit intrinsic
ClosedPublic

Authored by abdulras on Apr 24 2014, 11:41 AM.

Details

Reviewers
rnk
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.

Diff Detail

Event Timeline

abdulras updated this revision to Diff 8814.Apr 24 2014, 11:41 AM
abdulras retitled this revision from to CodeGen: implement __emit and __yield intrinsics.
abdulras updated this object.
abdulras edited the test plan for this revision. (Show Details)
abdulras added a reviewer: rnk.
abdulras set the repository for this revision to rL LLVM.
abdulras added a subscriber: Unknown Object (MLST).
abdulras updated this object.Apr 24 2014, 11:41 AM

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 updated this revision to Diff 8818.Apr 24 2014, 3:21 PM

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

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

rnk edited edge metadata.Apr 24 2014, 3:39 PM

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.

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

Tim.

@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 added a comment.Jun 17 2014, 3:43 PM
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.

abdulras updated this revision to Diff 16934.Dec 4 2014, 9:43 AM
abdulras retitled this revision from CodeGen: implement __emit and __yield intrinsics to CodeGen: implement __emit intrinsic.
abdulras updated this object.
abdulras edited edge metadata.
majnemer added inline comments.
lib/Basic/Builtins.cpp
58–61 ↗(On Diff #16934)

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

abdulras added inline comments.Dec 4 2014, 3:10 PM
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 added inline comments.Dec 4 2014, 3:30 PM
lib/Basic/Builtins.cpp
58–61 ↗(On Diff #16934)

Renaming MS_MODE to MS_COMPAT seems reasonable.

abdulras updated this revision to Diff 17010.Dec 5 2014, 5:11 PM

Address review comments from majnemer

rnk added a comment.Dec 15 2014, 6:20 PM

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.

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 updated this revision to Diff 17340.Dec 16 2014, 9:51 AM

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

rnk added a comment.Dec 16 2014, 10:43 AM

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 added inline comments.Dec 16 2014, 2:09 PM
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 updated this revision to Diff 17368.Dec 16 2014, 2:50 PM

Address rnk's comments.

rnk accepted this revision.Dec 16 2014, 3:14 PM
rnk edited edge metadata.

lgtm

lib/CodeGen/CGBuiltin.cpp
3180

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

This revision is now accepted and ready to land.Dec 16 2014, 3:14 PM
compnerd closed this revision.Dec 17 2014, 8:50 PM
compnerd added a subscriber: compnerd.

SVN r224438.