Page MenuHomePhabricator

CodeGen: improve ms instrincics support
ClosedPublic

Authored by abdulras on Jun 17 2014, 1:47 PM.

Details

Reviewers
whunt
rnk
Summary

Add support for _InterlockedCompareExchangePointer, _InterlockExchangePointer,
_InterlockExchange. These are available as a compiler intrinsic on ARM and x86.
These are used directly by the Windows SDK headers without use of the intrin
header.

Diff Detail

Event Timeline

abdulras updated this revision to Diff 10512.Jun 17 2014, 1:47 PM
abdulras retitled this revision from to CodeGen: improve ms instrincics support.
abdulras updated this object.
abdulras edited the test plan for this revision. (Show Details)
abdulras added reviewers: rnk, whunt.
abdulras set the repository for this revision to rL LLVM.
abdulras added subscribers: Unknown Object (MLST), compnerd.
rnk added inline comments.Jun 17 2014, 2:35 PM
lib/Sema/SemaChecking.cpp
139

I don't think these intrinsics need the special overload handling that the __sync intrinsics require. They should just need type checking that's already described by Builtins.def.

145–147

The AST should reflect the code the user wrote, and we shouldn't be doing this kind of rewrite. The overload resolution in the existing checking code is different, it's taking something that the user wrote and annotating it with all the implicit conversions that should occur.

This should have it's own lowering in CGBuiltin.cpp.

abdulras updated this revision to Diff 10568.Jun 18 2014, 8:31 AM
abdulras updated this object.

Address comments from rnk.

whunt added inline comments.Jun 18 2014, 10:22 AM
lib/Headers/Intrin.h
227 ↗(On Diff #10568)

I don't see why it was necessary to get rid of cdecl. Also, Now that the definition is gone it shouldn't be static inline__ anymore.

abdulras added inline comments.Jun 18 2014, 10:47 AM
lib/Headers/Intrin.h
227 ↗(On Diff #10568)

Getting rid of __cdecl is actually quite important since this header is not x86 specific. ARM does not honour __cdecl, and thus generates a warning.

Good catch about the static __inline__. I've become too accustomed to the LLVM style and didnt notice that.

abdulras updated this revision to Diff 10580.Jun 18 2014, 10:56 AM

Fix Intrin.h with minor oversights caught by Warren.

whunt accepted this revision.Jun 18 2014, 11:36 AM
whunt edited edge metadata.

lgtm

This revision is now accepted and ready to land.Jun 18 2014, 11:36 AM
rnk accepted this revision.Jun 18 2014, 11:41 AM
rnk edited edge metadata.

lgtm with some nits

include/clang/Basic/Builtins.def
690

MSVC doesn't provide this in 32-bit mode, but I think we can go ahead and do that anyway.

lib/CodeGen/CGBuiltin.cpp
1523

I'd like this to be factored better, but I can live with it as is.

1535

CGM.Int32Ty doesn't seem correct for x64, it should be IntType.

1537

MSDN says "Comparand" with an 'a'.

1540

MSDN says the 'Dest' is volatile, so I'd call Result->setVolatile(true) here.

abdulras added inline comments.Jun 18 2014, 12:53 PM
include/clang/Basic/Builtins.def
690

Id be willing to do a follow up change to disable this at the sema level in 32-bit x86 only if you like. This is available in 32-bit ARM.

lib/CodeGen/CGBuiltin.cpp
1535

Good catch!

1537

Done.

1540

Dest itself is volatile, the returned value is not. We dont strip volatility off of Destination. Oh, and I dont see a llvm::Value::setVolatile either.

rnk added inline comments.Jun 18 2014, 1:14 PM
include/clang/Basic/Builtins.def
690

No need. I always thought it was silly that they didn't go back and add this intrinsic for x86.

lib/CodeGen/CGBuiltin.cpp
1540

CreateAtomicCmpXchg should return an AtomicRMWInst* which can be made volatile. It's not the result that's volatile, it's the memory update.

abdulras added inline comments.Jun 18 2014, 1:49 PM
lib/CodeGen/CGBuiltin.cpp
1540

Done (thanks for the clarification!); although needed an additional extract value due to a recent change to the cmpxchg change.

abdulras closed this revision.Jun 19 2014, 8:23 AM