Skip to content

Commit c4ef805

Browse files
ostannardostannard
authored
[Clang] Re-write codegen for atomic_test_and_set and atomic_clear (llvm#121943)
Re-write the sema and codegen for the atomic_test_and_set and atomic_clear builtin functions to go via AtomicExpr, like the other atomic builtins do. This simplifies the code, because AtomicExpr already handles things like generating code for to dynamically select the memory ordering, which was duplicated for these builtins. This also fixes a few crash bugs, one when passing an integer to the pointer argument, and one when using an array. This also adds diagnostics for the memory orderings which are not valid for atomic_clear according to https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html, which were missing before. Fixes llvm#111293. This is a re-land of llvm#120449, modified to allow any non-const pointer type for the first argument.
1 parent 58be6fd commit c4ef805

File tree

7 files changed

+447
-165
lines changed

7 files changed

+447
-165
lines changed

‎clang/include/clang/Basic/Builtins.td‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1977,16 +1977,16 @@ def AtomicNandFetch : AtomicBuiltin {
19771977
let Prototype = "void(...)";
19781978
}
19791979

1980-
def AtomicTestAndSet : Builtin {
1980+
def AtomicTestAndSet : AtomicBuiltin {
19811981
let Spellings = ["__atomic_test_and_set"];
1982-
let Attributes = [NoThrow];
1983-
let Prototype = "bool(void volatile*, int)";
1982+
let Attributes = [NoThrow, CustomTypeChecking];
1983+
let Prototype = "bool(...)";
19841984
}
19851985

1986-
def AtomicClear : Builtin {
1986+
def AtomicClear : AtomicBuiltin {
19871987
let Spellings = ["__atomic_clear"];
1988-
let Attributes = [NoThrow];
1989-
let Prototype = "void(void volatile*, int)";
1988+
let Attributes = [NoThrow, CustomTypeChecking];
1989+
let Prototype = "void(...)";
19901990
}
19911991

19921992
def AtomicThreadFence : Builtin {

‎clang/lib/AST/Expr.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5077,6 +5077,8 @@ unsigned AtomicExpr::getNumSubExprs(AtomicOp Op) {
50775077
case AO__opencl_atomic_init:
50785078
case AO__c11_atomic_load:
50795079
case AO__atomic_load_n:
5080+
case AO__atomic_test_and_set:
5081+
case AO__atomic_clear:
50805082
return 2;
50815083

50825084
case AO__scoped_atomic_load_n:

‎clang/lib/CodeGen/CGAtomic.cpp‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -723,6 +723,24 @@ static void EmitAtomicOp(CodeGenFunction &CGF, AtomicExpr *E, Address Dest,
723723
case AtomicExpr::AO__scoped_atomic_fetch_nand:
724724
Op = llvm::AtomicRMWInst::Nand;
725725
break;
726+
727+
case AtomicExpr::AO__atomic_test_and_set: {
728+
llvm::AtomicRMWInst *RMWI =
729+
CGF.emitAtomicRMWInst(llvm::AtomicRMWInst::Xchg, Ptr,
730+
CGF.Builder.getInt8(1), Order, Scope, E);
731+
RMWI->setVolatile(E->isVolatile());
732+
llvm::Value *Result = CGF.Builder.CreateIsNotNull(RMWI, "tobool");
733+
CGF.Builder.CreateStore(Result, Dest);
734+
return;
735+
}
736+
737+
case AtomicExpr::AO__atomic_clear: {
738+
llvm::StoreInst *Store =
739+
CGF.Builder.CreateStore(CGF.Builder.getInt8(0), Ptr);
740+
Store->setAtomic(Order, Scope);
741+
Store->setVolatile(E->isVolatile());
742+
return;
743+
}
726744
}
727745

728746
llvm::Value *LoadVal1 = CGF.Builder.CreateLoad(Val1);
@@ -878,6 +896,8 @@ RValue CodeGenFunction::EmitAtomicExpr(AtomicExpr *E) {
878896
case AtomicExpr::AO__c11_atomic_load:
879897
case AtomicExpr::AO__opencl_atomic_load:
880898
case AtomicExpr::AO__hip_atomic_load:
899+
case AtomicExpr::AO__atomic_test_and_set:
900+
case AtomicExpr::AO__atomic_clear:
881901
break;
882902

883903
case AtomicExpr::AO__atomic_load:
@@ -1200,6 +1220,8 @@ RValue CodeGenFunction::EmitAtomicExpr(AtomicExpr *E) {
12001220
case AtomicExpr::AO__opencl_atomic_fetch_max:
12011221
case AtomicExpr::AO__scoped_atomic_fetch_max:
12021222
case AtomicExpr::AO__scoped_atomic_max_fetch:
1223+
case AtomicExpr::AO__atomic_test_and_set:
1224+
case AtomicExpr::AO__atomic_clear:
12031225
llvm_unreachable("Integral atomic operations always become atomicrmw!");
12041226
}
12051227

@@ -1239,7 +1261,8 @@ RValue CodeGenFunction::EmitAtomicExpr(AtomicExpr *E) {
12391261
E->getOp() == AtomicExpr::AO__atomic_store ||
12401262
E->getOp() == AtomicExpr::AO__atomic_store_n ||
12411263
E->getOp() == AtomicExpr::AO__scoped_atomic_store ||
1242-
E->getOp() == AtomicExpr::AO__scoped_atomic_store_n;
1264+
E->getOp() == AtomicExpr::AO__scoped_atomic_store_n ||
1265+
E->getOp() == AtomicExpr::AO__atomic_clear;
12431266
bool IsLoad = E->getOp() == AtomicExpr::AO__c11_atomic_load ||
12441267
E->getOp() == AtomicExpr::AO__opencl_atomic_load ||
12451268
E->getOp() == AtomicExpr::AO__hip_atomic_load ||

‎clang/lib/CodeGen/CGBuiltin.cpp‎

Lines changed: 0 additions & 141 deletions
Original file line numberDiff line numberDiff line change
@@ -5128,147 +5128,6 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
51285128
ReturnValueSlot(), Args);
51295129
}
51305130

5131-
case Builtin::BI__atomic_test_and_set: {
5132-
// Look at the argument type to determine whether this is a volatile
5133-
// operation. The parameter type is always volatile.
5134-
QualType PtrTy = E->getArg(0)->IgnoreImpCasts()->getType();
5135-
bool Volatile =
5136-
PtrTy->castAs<PointerType>()->getPointeeType().isVolatileQualified();
5137-
5138-
Address Ptr =
5139-
EmitPointerWithAlignment(E->getArg(0)).withElementType(Int8Ty);
5140-
5141-
Value *NewVal = Builder.getInt8(1);
5142-
Value *Order = EmitScalarExpr(E->getArg(1));
5143-
if (isa<llvm::ConstantInt>(Order)) {
5144-
int ord = cast<llvm::ConstantInt>(Order)->getZExtValue();
5145-
AtomicRMWInst *Result = nullptr;
5146-
switch (ord) {
5147-
case 0: // memory_order_relaxed
5148-
default: // invalid order
5149-
Result = Builder.CreateAtomicRMW(llvm::AtomicRMWInst::Xchg, Ptr, NewVal,
5150-
llvm::AtomicOrdering::Monotonic);
5151-
break;
5152-
case 1: // memory_order_consume
5153-
case 2: // memory_order_acquire
5154-
Result = Builder.CreateAtomicRMW(llvm::AtomicRMWInst::Xchg, Ptr, NewVal,
5155-
llvm::AtomicOrdering::Acquire);
5156-
break;
5157-
case 3: // memory_order_release
5158-
Result = Builder.CreateAtomicRMW(llvm::AtomicRMWInst::Xchg, Ptr, NewVal,
5159-
llvm::AtomicOrdering::Release);
5160-
break;
5161-
case 4: // memory_order_acq_rel
5162-
5163-
Result = Builder.CreateAtomicRMW(llvm::AtomicRMWInst::Xchg, Ptr, NewVal,
5164-
llvm::AtomicOrdering::AcquireRelease);
5165-
break;
5166-
case 5: // memory_order_seq_cst
5167-
Result = Builder.CreateAtomicRMW(
5168-
llvm::AtomicRMWInst::Xchg, Ptr, NewVal,
5169-
llvm::AtomicOrdering::SequentiallyConsistent);
5170-
break;
5171-
}
5172-
Result->setVolatile(Volatile);
5173-
return RValue::get(Builder.CreateIsNotNull(Result, "tobool"));
5174-
}
5175-
5176-
llvm::BasicBlock *ContBB = createBasicBlock("atomic.continue", CurFn);
5177-
5178-
llvm::BasicBlock *BBs[5] = {
5179-
createBasicBlock("monotonic", CurFn),
5180-
createBasicBlock("acquire", CurFn),
5181-
createBasicBlock("release", CurFn),
5182-
createBasicBlock("acqrel", CurFn),
5183-
createBasicBlock("seqcst", CurFn)
5184-
};
5185-
llvm::AtomicOrdering Orders[5] = {
5186-
llvm::AtomicOrdering::Monotonic, llvm::AtomicOrdering::Acquire,
5187-
llvm::AtomicOrdering::Release, llvm::AtomicOrdering::AcquireRelease,
5188-
llvm::AtomicOrdering::SequentiallyConsistent};
5189-
5190-
Order = Builder.CreateIntCast(Order, Builder.getInt32Ty(), false);
5191-
llvm::SwitchInst *SI = Builder.CreateSwitch(Order, BBs[0]);
5192-
5193-
Builder.SetInsertPoint(ContBB);
5194-
PHINode *Result = Builder.CreatePHI(Int8Ty, 5, "was_set");
5195-
5196-
for (unsigned i = 0; i < 5; ++i) {
5197-
Builder.SetInsertPoint(BBs[i]);
5198-
AtomicRMWInst *RMW = Builder.CreateAtomicRMW(llvm::AtomicRMWInst::Xchg,
5199-
Ptr, NewVal, Orders[i]);
5200-
RMW->setVolatile(Volatile);
5201-
Result->addIncoming(RMW, BBs[i]);
5202-
Builder.CreateBr(ContBB);
5203-
}
5204-
5205-
SI->addCase(Builder.getInt32(0), BBs[0]);
5206-
SI->addCase(Builder.getInt32(1), BBs[1]);
5207-
SI->addCase(Builder.getInt32(2), BBs[1]);
5208-
SI->addCase(Builder.getInt32(3), BBs[2]);
5209-
SI->addCase(Builder.getInt32(4), BBs[3]);
5210-
SI->addCase(Builder.getInt32(5), BBs[4]);
5211-
5212-
Builder.SetInsertPoint(ContBB);
5213-
return RValue::get(Builder.CreateIsNotNull(Result, "tobool"));
5214-
}
5215-
5216-
case Builtin::BI__atomic_clear: {
5217-
QualType PtrTy = E->getArg(0)->IgnoreImpCasts()->getType();
5218-
bool Volatile =
5219-
PtrTy->castAs<PointerType>()->getPointeeType().isVolatileQualified();
5220-
5221-
Address Ptr = EmitPointerWithAlignment(E->getArg(0));
5222-
Ptr = Ptr.withElementType(Int8Ty);
5223-
Value *NewVal = Builder.getInt8(0);
5224-
Value *Order = EmitScalarExpr(E->getArg(1));
5225-
if (isa<llvm::ConstantInt>(Order)) {
5226-
int ord = cast<llvm::ConstantInt>(Order)->getZExtValue();
5227-
StoreInst *Store = Builder.CreateStore(NewVal, Ptr, Volatile);
5228-
switch (ord) {
5229-
case 0: // memory_order_relaxed
5230-
default: // invalid order
5231-
Store->setOrdering(llvm::AtomicOrdering::Monotonic);
5232-
break;
5233-
case 3: // memory_order_release
5234-
Store->setOrdering(llvm::AtomicOrdering::Release);
5235-
break;
5236-
case 5: // memory_order_seq_cst
5237-
Store->setOrdering(llvm::AtomicOrdering::SequentiallyConsistent);
5238-
break;
5239-
}
5240-
return RValue::get(nullptr);
5241-
}
5242-
5243-
llvm::BasicBlock *ContBB = createBasicBlock("atomic.continue", CurFn);
5244-
5245-
llvm::BasicBlock *BBs[3] = {
5246-
createBasicBlock("monotonic", CurFn),
5247-
createBasicBlock("release", CurFn),
5248-
createBasicBlock("seqcst", CurFn)
5249-
};
5250-
llvm::AtomicOrdering Orders[3] = {
5251-
llvm::AtomicOrdering::Monotonic, llvm::AtomicOrdering::Release,
5252-
llvm::AtomicOrdering::SequentiallyConsistent};
5253-
5254-
Order = Builder.CreateIntCast(Order, Builder.getInt32Ty(), false);
5255-
llvm::SwitchInst *SI = Builder.CreateSwitch(Order, BBs[0]);
5256-
5257-
for (unsigned i = 0; i < 3; ++i) {
5258-
Builder.SetInsertPoint(BBs[i]);
5259-
StoreInst *Store = Builder.CreateStore(NewVal, Ptr, Volatile);
5260-
Store->setOrdering(Orders[i]);
5261-
Builder.CreateBr(ContBB);
5262-
}
5263-
5264-
SI->addCase(Builder.getInt32(0), BBs[0]);
5265-
SI->addCase(Builder.getInt32(3), BBs[1]);
5266-
SI->addCase(Builder.getInt32(5), BBs[2]);
5267-
5268-
Builder.SetInsertPoint(ContBB);
5269-
return RValue::get(nullptr);
5270-
}
5271-
52725131
case Builtin::BI__atomic_thread_fence:
52735132
case Builtin::BI__atomic_signal_fence:
52745133
case Builtin::BI__c11_atomic_thread_fence:

‎clang/lib/Sema/SemaChecking.cpp‎

Lines changed: 50 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -3634,6 +3634,7 @@ static bool isValidOrderingForOp(int64_t Ordering, AtomicExpr::AtomicOp Op) {
36343634
case AtomicExpr::AO__atomic_store_n:
36353635
case AtomicExpr::AO__scoped_atomic_store:
36363636
case AtomicExpr::AO__scoped_atomic_store_n:
3637+
case AtomicExpr::AO__atomic_clear:
36373638
return OrderingCABI != llvm::AtomicOrderingCABI::consume &&
36383639
OrderingCABI != llvm::AtomicOrderingCABI::acquire &&
36393640
OrderingCABI != llvm::AtomicOrderingCABI::acq_rel;
@@ -3686,12 +3687,18 @@ ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
36863687
C11CmpXchg,
36873688

36883689
// bool __atomic_compare_exchange(A *, C *, CP, bool, int, int)
3689-
GNUCmpXchg
3690+
GNUCmpXchg,
3691+
3692+
// bool __atomic_test_and_set(A *, int)
3693+
TestAndSetByte,
3694+
3695+
// void __atomic_clear(A *, int)
3696+
ClearByte,
36903697
} Form = Init;
36913698

3692-
const unsigned NumForm = GNUCmpXchg + 1;
3693-
const unsigned NumArgs[] = { 2, 2, 3, 3, 3, 3, 4, 5, 6 };
3694-
const unsigned NumVals[] = { 1, 0, 1, 1, 1, 1, 2, 2, 3 };
3699+
const unsigned NumForm = ClearByte + 1;
3700+
const unsigned NumArgs[] = {2, 2, 3, 3, 3, 3, 4, 5, 6, 2, 2};
3701+
const unsigned NumVals[] = {1, 0, 1, 1, 1, 1, 2, 2, 3, 0, 0};
36953702
// where:
36963703
// C is an appropriate type,
36973704
// A is volatile _Atomic(C) for __c11 builtins and is C for GNU builtins,
@@ -3852,6 +3859,14 @@ ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
38523859
case AtomicExpr::AO__scoped_atomic_compare_exchange_n:
38533860
Form = GNUCmpXchg;
38543861
break;
3862+
3863+
case AtomicExpr::AO__atomic_test_and_set:
3864+
Form = TestAndSetByte;
3865+
break;
3866+
3867+
case AtomicExpr::AO__atomic_clear:
3868+
Form = ClearByte;
3869+
break;
38553870
}
38563871

38573872
unsigned AdjustedNumArgs = NumArgs[Form];
@@ -3911,14 +3926,28 @@ ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
39113926
}
39123927
}
39133928

3914-
// Pointer to object of size zero is not allowed.
3915-
if (RequireCompleteType(Ptr->getBeginLoc(), AtomTy,
3916-
diag::err_incomplete_type))
3917-
return ExprError();
3918-
if (Context.getTypeInfoInChars(AtomTy).Width.isZero()) {
3919-
Diag(ExprRange.getBegin(), diag::err_atomic_builtin_must_be_pointer)
3920-
<< Ptr->getType() << 1 << Ptr->getSourceRange();
3921-
return ExprError();
3929+
if (Form != TestAndSetByte && Form != ClearByte) {
3930+
// Pointer to object of size zero is not allowed.
3931+
if (RequireCompleteType(Ptr->getBeginLoc(), AtomTy,
3932+
diag::err_incomplete_type))
3933+
return ExprError();
3934+
3935+
if (Context.getTypeInfoInChars(AtomTy).Width.isZero()) {
3936+
Diag(ExprRange.getBegin(), diag::err_atomic_builtin_must_be_pointer)
3937+
<< Ptr->getType() << 1 << Ptr->getSourceRange();
3938+
return ExprError();
3939+
}
3940+
} else {
3941+
// The __atomic_clear and __atomic_test_and_set intrinsics accept any
3942+
// non-const pointer type, including void* and pointers to incomplete
3943+
// structs, but only access the first byte.
3944+
AtomTy = Context.CharTy;
3945+
AtomTy = AtomTy.withCVRQualifiers(
3946+
pointerType->getPointeeType().getCVRQualifiers());
3947+
QualType PointerQT = Context.getPointerType(AtomTy);
3948+
pointerType = PointerQT->getAs<PointerType>();
3949+
Ptr = ImpCastExprToType(Ptr, PointerQT, CK_BitCast).get();
3950+
ValType = AtomTy;
39223951
}
39233952

39243953
// For an arithmetic operation, the implied arithmetic must be well-formed.
@@ -3997,10 +4026,10 @@ ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
39974026
ValType.removeLocalVolatile();
39984027
ValType.removeLocalConst();
39994028
QualType ResultType = ValType;
4000-
if (Form == Copy || Form == LoadCopy || Form == GNUXchg ||
4001-
Form == Init)
4029+
if (Form == Copy || Form == LoadCopy || Form == GNUXchg || Form == Init ||
4030+
Form == ClearByte)
40024031
ResultType = Context.VoidTy;
4003-
else if (Form == C11CmpXchg || Form == GNUCmpXchg)
4032+
else if (Form == C11CmpXchg || Form == GNUCmpXchg || Form == TestAndSetByte)
40044033
ResultType = Context.BoolTy;
40054034

40064035
// The type of a parameter passed 'by value'. In the GNU atomics, such
@@ -4045,6 +4074,10 @@ ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
40454074
APIOrderedArgs.push_back(Args[1]); // Order
40464075
APIOrderedArgs.push_back(Args[3]); // OrderFail
40474076
break;
4077+
case TestAndSetByte:
4078+
case ClearByte:
4079+
APIOrderedArgs.push_back(Args[1]); // Order
4080+
break;
40484081
}
40494082
} else
40504083
APIOrderedArgs.append(Args.begin(), Args.end());
@@ -4130,6 +4163,8 @@ ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
41304163
SubExprs.push_back(APIOrderedArgs[1]); // Val1
41314164
break;
41324165
case Load:
4166+
case TestAndSetByte:
4167+
case ClearByte:
41334168
SubExprs.push_back(APIOrderedArgs[1]); // Order
41344169
break;
41354170
case LoadCopy:

0 commit comments

Comments
 (0)