================
Comment at: lib/CodeGen/CGAtomic.cpp:1011
@@ +1010,3 @@
+ AtomicInfo AI(*this, LV);
+ return CGM.getCodeGenOpts().MSVolatile && !AI.shouldUseLibcall() &&
+ (LV.isVolatile() || hasVolatileMember(LV.getType()));
----------------
dvyukov wrote:
> I don't get the !AI.shouldUseLibcall() part.
> If the atomic operation would use a libcall, then you don't turn volatile
> access into atomic? Why? How is it related to libcalls?
`shouldUseLibcall` means that than atomic operation is not suitably aligned or
sized to be lowered to a `load atomic` or `store atomic` instruction. These
conditions are identical to the conditions that MSVC uses to say that a
volatile load or store will also have acquire or release semantics.
================
Comment at: lib/CodeGen/CGAtomic.cpp:1017
@@ +1016,3 @@
+ return CGM.getCodeGenOpts().MSVolatile &&
+ getContext().getTargetInfo().hasBuiltinAtomic(
+ getContext().getTypeSize(Ty), getContext().getTypeAlign(Ty));
----------------
dvyukov wrote:
> Same here. How presence of builtin atomic affects this?
As I mentioned earlier, not all volatile loads or stores will turn into acquire
or release loads and stores.
================
Comment at: lib/CodeGen/CGAtomic.cpp:1018
@@ +1017,3 @@
+ getContext().getTargetInfo().hasBuiltinAtomic(
+ getContext().getTypeSize(Ty), getContext().getTypeAlign(Ty));
+}
----------------
dvyukov wrote:
> In volatileIsAtomic you check that the value is volatile, but here you don't.
> Please make then consistent. Either check in both or don't check in both.
I can't check it here because the type being passed in might not be qualified
with volatile, it might be a record with a volatile field. I'd rather not make
everyone who calls the other overload to have to manually check
`LV.isVolatile() || hasVolatileMember(LV.getType())`.
I'll just rename one of these to make it clear what it does.
================
Comment at: lib/CodeGen/CGExpr.cpp:1141
@@ -1138,3 +1140,3 @@
// Atomic operations have to be done on integral types.
- if (Ty->isAtomicType()) {
+ if (Ty->isAtomicType() || VolatileIsAtomic) {
LValue lvalue = LValue::MakeAddr(Addr, Ty,
----------------
dvyukov wrote:
> Won't this be overly fragile?
> MS semantics effectively turn volatile variables into atomic variables. So
> the current isAtomicType becomes effectively useless, and whoever uses
> isAtomicType must also remember to add volatileIsAtomic. And there is nothing
> that reminds of that. Ty->isAtomicType() looks pretty much like what you need.
> It would be less fragile if you fix the isAtomic predicate itself.
> But I know very little about llvm internals, maybe it is not possible, or a
> bad idea for some other reason. I don't know.
I don't think that is semantically correct. `isAtomicType()` is used for types
which are "qualified" with `_Atomic`.
Moreover, we would still need to be able to determine the different from a real
`_Atomic` qualified type and a MS `volatile` type in order to determine the
memory operation's order.
================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:836
@@ -834,1 +835,3 @@
+ IsSeqCst ? llvm::SequentiallyConsistent
+ : llvm::Unordered);
// OpenMP, 2.12.6, atomic Construct
----------------
ABataev wrote:
> I'd say that if IsSeqCst is false, Atomic Ordering must be Monotonic rather
> than Unordered.
> 1. According to OpenMP 4.0 "non-sequentially consistent atomic construct has
> the same semantics as a
> memory_order_relaxed atomic operation in C++11/C11.".
> 2. According to http://llvm.org/docs/Atomics.html "Monotonic... This
> corresponds to the C++11/C11 memory_order_relaxed"
Good catch, will do.
================
Comment at: lib/CodeGen/CodeGenFunction.h:2159
@@ +2158,3 @@
+ } else {
+ IsSeqCst = false;
+ IsVolatile = true;
----------------
dvyukov wrote:
> Here you use IsSeqCst, but in EmitAtomicStore you use AO.
> Please make them consistent.
Sure, will do.
http://reviews.llvm.org/D7580
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
