This is an archive of the discontinued LLVM Phabricator instance.

[MS] Implement __iso_volatile loads/stores as builtins
ClosedPublic

Authored by mstorsjo on Sep 27 2016, 1:29 PM.

Details

Summary

These are supposed to produce the same as normal volatile pointer loads/stores. When -volatile:ms is specified, normal volatile pointers are forced to have atomic semantics (as is the default on x86 in MSVC mode). In that case, these builtins should still produce non-atomic volatile loads/stores without acquire/release semantics, which the new test verifies.

These are only available on ARM (and on AArch64, although clang doesn't support AArch64/Windows yet).

This implements what is missing for PR30394, making it possible to compile C++ for ARM in MSVC mode with MSVC headers.

Diff Detail

Event Timeline

mstorsjo updated this revision to Diff 72702.Sep 27 2016, 1:29 PM
mstorsjo retitled this revision from to Headers: Add iso_volatile load/store intrinsics.
mstorsjo updated this object.
mstorsjo added a reviewer: majnemer.
mstorsjo added a subscriber: cfe-commits.
majnemer edited edge metadata.Sep 27 2016, 2:36 PM

IMO, this should be implemented in clang CodeGen so that we don't get extra acquire/release barriers with /volatile:ms but that might be overkill; feel free to disregard this.

mstorsjo updated this revision to Diff 72782.Sep 28 2016, 2:30 AM
mstorsjo retitled this revision from Headers: Add iso_volatile load/store intrinsics to [MS] Implement __iso_volatile loads/stores as builtins.
mstorsjo updated this object.
mstorsjo edited edge metadata.

Changed to implement it as builtins, as requested. I had to put this at the bottom in EmitBuiltinExpr (which returns RValues) instead of in EmitARMBuiltinExpr (which returns Value*), since the returned Value* for stores is nullptr. A nullptr return value from EmitTargetBuiltinExpr indicates that it wasn't handled, this triggered errors about the store builtins being unsupported.

majnemer added inline comments.Sep 29 2016, 11:21 PM
lib/CodeGen/CGBuiltin.cpp
2597–2611

I think you could sink this into EmitARMBuiltinExpr. You don't really need or want the fancy CGF machinery here. I'd just return the result of CreateAlignedStore/CreateAlignedLoad which should handle the EmitTargetBuiltinExpr issue.

mstorsjo updated this revision to Diff 72988.Sep 29 2016, 11:58 PM

Implemented using CreateAlignedStore and CreateAlignedLoad instead.

I'm less confident in what this actually does though - e.g. is the CreateBitCast part necessary at all? I just based this on code I found in the same file calling CreateAlignedStore. (If I comment out the CreateBitCast line, it still works just as fine, at least for the unit test - but what about actual use?)

Compared to the previous patch, the generated IR now lacks a !tbaa !3 marker at the end - is that an issue?

majnemer accepted this revision.Sep 30 2016, 10:51 AM
majnemer edited edge metadata.

LGTM

test/CodeGen/ms-volatile-arm.c
3

You don't need -fms-volatile.

This revision is now accepted and ready to land.Sep 30 2016, 10:51 AM
mstorsjo added inline comments.Sep 30 2016, 11:09 AM
test/CodeGen/ms-volatile-arm.c
3

Well, originally, the point was to clarify that these volatile stores end up without atomic semantics, regardless of whether -volatile:ms has been specified. The original version of this patch (with an inline implementation in intrin.h, just using a normal volatile pointer) wouldn't pass this test, but would pass if I remove this option. But if you prefer, I can leave it out.

majnemer added inline comments.Sep 30 2016, 11:40 AM
test/CodeGen/ms-volatile-arm.c
3

Makes sense.

This revision was automatically updated to reflect the committed changes.