Skip to content

[Clang] Re-write codegen for atomic_test_and_set and atomic_clear #121943

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 22, 2025
Merged

[Clang] Re-write codegen for atomic_test_and_set and atomic_clear #121943

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a69ca9
[Clang] Re-write codegen for atomic_test_and_set and atomic_clear (#1…
ostannard Dec 19, 2024
e8b924b
Allow any non-const pointer type
ostannard Jan 7, 2025
473b0da
Review comments
ostannard Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions clang/include/clang/Basic/Builtins.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1977,16 +1977,16 @@ def AtomicNandFetch : AtomicBuiltin {
let Prototype = "void(...)";
}

def AtomicTestAndSet : Builtin {
def AtomicTestAndSet : AtomicBuiltin {
let Spellings = ["__atomic_test_and_set"];
let Attributes = [NoThrow];
let Prototype = "bool(void volatile*, int)";
let Attributes = [NoThrow, CustomTypeChecking];
let Prototype = "void(...)";
Copy link
Member

@jyknight jyknight Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe doesn't matter, but shouldn't this be bool(...)?

Copy link
Collaborator Author

@ostannard ostannard Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

def AtomicClear : Builtin {
def AtomicClear : AtomicBuiltin {
let Spellings = ["__atomic_clear"];
let Attributes = [NoThrow];
let Prototype = "void(void volatile*, int)";
let Attributes = [NoThrow, CustomTypeChecking];
let Prototype = "void(...)";
}

def AtomicThreadFence : Builtin {
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/AST/Expr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5070,6 +5070,8 @@ unsigned AtomicExpr::getNumSubExprs(AtomicOp Op) {
case AO__opencl_atomic_init:
case AO__c11_atomic_load:
case AO__atomic_load_n:
case AO__atomic_test_and_set:
case AO__atomic_clear:
return 2;

case AO__scoped_atomic_load_n:
Expand Down
25 changes: 24 additions & 1 deletion clang/lib/CodeGen/CGAtomic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -723,6 +723,24 @@ static void EmitAtomicOp(CodeGenFunction &CGF, AtomicExpr *E, Address Dest,
case AtomicExpr::AO__scoped_atomic_fetch_nand:
Op = llvm::AtomicRMWInst::Nand;
break;

case AtomicExpr::AO__atomic_test_and_set: {
llvm::AtomicRMWInst *RMWI =
CGF.emitAtomicRMWInst(llvm::AtomicRMWInst::Xchg, Ptr,
CGF.Builder.getInt8(1), Order, Scope, E);
RMWI->setVolatile(E->isVolatile());
llvm::Value *Result = CGF.Builder.CreateIsNotNull(RMWI, "tobool");
CGF.Builder.CreateStore(Result, Dest);
return;
}

case AtomicExpr::AO__atomic_clear: {
llvm::StoreInst *Store =
CGF.Builder.CreateStore(CGF.Builder.getInt8(0), Ptr);
Store->setAtomic(Order, Scope);
Store->setVolatile(E->isVolatile());
return;
}
}

llvm::Value *LoadVal1 = CGF.Builder.CreateLoad(Val1);
Expand Down Expand Up @@ -878,6 +896,8 @@ RValue CodeGenFunction::EmitAtomicExpr(AtomicExpr *E) {
case AtomicExpr::AO__c11_atomic_load:
case AtomicExpr::AO__opencl_atomic_load:
case AtomicExpr::AO__hip_atomic_load:
case AtomicExpr::AO__atomic_test_and_set:
case AtomicExpr::AO__atomic_clear:
break;

case AtomicExpr::AO__atomic_load:
Expand Down Expand Up @@ -1200,6 +1220,8 @@ RValue CodeGenFunction::EmitAtomicExpr(AtomicExpr *E) {
case AtomicExpr::AO__opencl_atomic_fetch_max:
case AtomicExpr::AO__scoped_atomic_fetch_max:
case AtomicExpr::AO__scoped_atomic_max_fetch:
case AtomicExpr::AO__atomic_test_and_set:
case AtomicExpr::AO__atomic_clear:
llvm_unreachable("Integral atomic operations always become atomicrmw!");
}

Expand Down Expand Up @@ -1239,7 +1261,8 @@ RValue CodeGenFunction::EmitAtomicExpr(AtomicExpr *E) {
E->getOp() == AtomicExpr::AO__atomic_store ||
E->getOp() == AtomicExpr::AO__atomic_store_n ||
E->getOp() == AtomicExpr::AO__scoped_atomic_store ||
E->getOp() == AtomicExpr::AO__scoped_atomic_store_n;
E->getOp() == AtomicExpr::AO__scoped_atomic_store_n ||
E->getOp() == AtomicExpr::AO__atomic_clear;
bool IsLoad = E->getOp() == AtomicExpr::AO__c11_atomic_load ||
E->getOp() == AtomicExpr::AO__opencl_atomic_load ||
E->getOp() == AtomicExpr::AO__hip_atomic_load ||
Expand Down
141 changes: 0 additions & 141 deletions clang/lib/CodeGen/CGBuiltin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5139,147 +5139,6 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
ReturnValueSlot(), Args);
}

case Builtin::BI__atomic_test_and_set: {
// Look at the argument type to determine whether this is a volatile
// operation. The parameter type is always volatile.
QualType PtrTy = E->getArg(0)->IgnoreImpCasts()->getType();
bool Volatile =
PtrTy->castAs<PointerType>()->getPointeeType().isVolatileQualified();

Address Ptr =
EmitPointerWithAlignment(E->getArg(0)).withElementType(Int8Ty);

Value *NewVal = Builder.getInt8(1);
Value *Order = EmitScalarExpr(E->getArg(1));
if (isa<llvm::ConstantInt>(Order)) {
int ord = cast<llvm::ConstantInt>(Order)->getZExtValue();
AtomicRMWInst *Result = nullptr;
switch (ord) {
case 0: // memory_order_relaxed
default: // invalid order
Result = Builder.CreateAtomicRMW(llvm::AtomicRMWInst::Xchg, Ptr, NewVal,
llvm::AtomicOrdering::Monotonic);
break;
case 1: // memory_order_consume
case 2: // memory_order_acquire
Result = Builder.CreateAtomicRMW(llvm::AtomicRMWInst::Xchg, Ptr, NewVal,
llvm::AtomicOrdering::Acquire);
break;
case 3: // memory_order_release
Result = Builder.CreateAtomicRMW(llvm::AtomicRMWInst::Xchg, Ptr, NewVal,
llvm::AtomicOrdering::Release);
break;
case 4: // memory_order_acq_rel

Result = Builder.CreateAtomicRMW(llvm::AtomicRMWInst::Xchg, Ptr, NewVal,
llvm::AtomicOrdering::AcquireRelease);
break;
case 5: // memory_order_seq_cst
Result = Builder.CreateAtomicRMW(
llvm::AtomicRMWInst::Xchg, Ptr, NewVal,
llvm::AtomicOrdering::SequentiallyConsistent);
break;
}
Result->setVolatile(Volatile);
return RValue::get(Builder.CreateIsNotNull(Result, "tobool"));
}

llvm::BasicBlock *ContBB = createBasicBlock("atomic.continue", CurFn);

llvm::BasicBlock *BBs[5] = {
createBasicBlock("monotonic", CurFn),
createBasicBlock("acquire", CurFn),
createBasicBlock("release", CurFn),
createBasicBlock("acqrel", CurFn),
createBasicBlock("seqcst", CurFn)
};
llvm::AtomicOrdering Orders[5] = {
llvm::AtomicOrdering::Monotonic, llvm::AtomicOrdering::Acquire,
llvm::AtomicOrdering::Release, llvm::AtomicOrdering::AcquireRelease,
llvm::AtomicOrdering::SequentiallyConsistent};

Order = Builder.CreateIntCast(Order, Builder.getInt32Ty(), false);
llvm::SwitchInst *SI = Builder.CreateSwitch(Order, BBs[0]);

Builder.SetInsertPoint(ContBB);
PHINode *Result = Builder.CreatePHI(Int8Ty, 5, "was_set");

for (unsigned i = 0; i < 5; ++i) {
Builder.SetInsertPoint(BBs[i]);
AtomicRMWInst *RMW = Builder.CreateAtomicRMW(llvm::AtomicRMWInst::Xchg,
Ptr, NewVal, Orders[i]);
RMW->setVolatile(Volatile);
Result->addIncoming(RMW, BBs[i]);
Builder.CreateBr(ContBB);
}

SI->addCase(Builder.getInt32(0), BBs[0]);
SI->addCase(Builder.getInt32(1), BBs[1]);
SI->addCase(Builder.getInt32(2), BBs[1]);
SI->addCase(Builder.getInt32(3), BBs[2]);
SI->addCase(Builder.getInt32(4), BBs[3]);
SI->addCase(Builder.getInt32(5), BBs[4]);

Builder.SetInsertPoint(ContBB);
return RValue::get(Builder.CreateIsNotNull(Result, "tobool"));
}

case Builtin::BI__atomic_clear: {
QualType PtrTy = E->getArg(0)->IgnoreImpCasts()->getType();
bool Volatile =
PtrTy->castAs<PointerType>()->getPointeeType().isVolatileQualified();

Address Ptr = EmitPointerWithAlignment(E->getArg(0));
Ptr = Ptr.withElementType(Int8Ty);
Value *NewVal = Builder.getInt8(0);
Value *Order = EmitScalarExpr(E->getArg(1));
if (isa<llvm::ConstantInt>(Order)) {
int ord = cast<llvm::ConstantInt>(Order)->getZExtValue();
StoreInst *Store = Builder.CreateStore(NewVal, Ptr, Volatile);
switch (ord) {
case 0: // memory_order_relaxed
default: // invalid order
Store->setOrdering(llvm::AtomicOrdering::Monotonic);
break;
case 3: // memory_order_release
Store->setOrdering(llvm::AtomicOrdering::Release);
break;
case 5: // memory_order_seq_cst
Store->setOrdering(llvm::AtomicOrdering::SequentiallyConsistent);
break;
}
return RValue::get(nullptr);
}

llvm::BasicBlock *ContBB = createBasicBlock("atomic.continue", CurFn);

llvm::BasicBlock *BBs[3] = {
createBasicBlock("monotonic", CurFn),
createBasicBlock("release", CurFn),
createBasicBlock("seqcst", CurFn)
};
llvm::AtomicOrdering Orders[3] = {
llvm::AtomicOrdering::Monotonic, llvm::AtomicOrdering::Release,
llvm::AtomicOrdering::SequentiallyConsistent};

Order = Builder.CreateIntCast(Order, Builder.getInt32Ty(), false);
llvm::SwitchInst *SI = Builder.CreateSwitch(Order, BBs[0]);

for (unsigned i = 0; i < 3; ++i) {
Builder.SetInsertPoint(BBs[i]);
StoreInst *Store = Builder.CreateStore(NewVal, Ptr, Volatile);
Store->setOrdering(Orders[i]);
Builder.CreateBr(ContBB);
}

SI->addCase(Builder.getInt32(0), BBs[0]);
SI->addCase(Builder.getInt32(3), BBs[1]);
SI->addCase(Builder.getInt32(5), BBs[2]);

Builder.SetInsertPoint(ContBB);
return RValue::get(nullptr);
}

case Builtin::BI__atomic_thread_fence:
case Builtin::BI__atomic_signal_fence:
case Builtin::BI__c11_atomic_thread_fence:
Expand Down
72 changes: 55 additions & 17 deletions clang/lib/Sema/SemaChecking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3634,6 +3634,7 @@ static bool isValidOrderingForOp(int64_t Ordering, AtomicExpr::AtomicOp Op) {
case AtomicExpr::AO__atomic_store_n:
case AtomicExpr::AO__scoped_atomic_store:
case AtomicExpr::AO__scoped_atomic_store_n:
case AtomicExpr::AO__atomic_clear:
return OrderingCABI != llvm::AtomicOrderingCABI::consume &&
OrderingCABI != llvm::AtomicOrderingCABI::acquire &&
OrderingCABI != llvm::AtomicOrderingCABI::acq_rel;
Expand Down Expand Up @@ -3686,12 +3687,18 @@ ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
C11CmpXchg,

// bool __atomic_compare_exchange(A *, C *, CP, bool, int, int)
GNUCmpXchg
GNUCmpXchg,

// bool __atomic_test_and_set(A *, int)
TestAndSet,
Copy link
Member

@jyknight jyknight Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For increased clarity, can we rename these new enum values TestAndSetByte and ClearByte.

Copy link
Collaborator Author

@ostannard ostannard Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


// void __atomic_clear(A *, int)
Clear,
} Form = Init;

const unsigned NumForm = GNUCmpXchg + 1;
const unsigned NumArgs[] = { 2, 2, 3, 3, 3, 3, 4, 5, 6 };
const unsigned NumVals[] = { 1, 0, 1, 1, 1, 1, 2, 2, 3 };
const unsigned NumForm = Clear + 1;
const unsigned NumArgs[] = {2, 2, 3, 3, 3, 3, 4, 5, 6, 2, 2};
const unsigned NumVals[] = {1, 0, 1, 1, 1, 1, 2, 2, 3, 0, 0};
// where:
// C is an appropriate type,
// A is volatile _Atomic(C) for __c11 builtins and is C for GNU builtins,
Expand Down Expand Up @@ -3852,6 +3859,14 @@ ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
case AtomicExpr::AO__scoped_atomic_compare_exchange_n:
Form = GNUCmpXchg;
break;

case AtomicExpr::AO__atomic_test_and_set:
Form = TestAndSet;
break;

case AtomicExpr::AO__atomic_clear:
Form = Clear;
break;
}

unsigned AdjustedNumArgs = NumArgs[Form];
Expand Down Expand Up @@ -3911,14 +3926,31 @@ ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
}
}

// Pointer to object of size zero is not allowed.
if (RequireCompleteType(Ptr->getBeginLoc(), AtomTy,
diag::err_incomplete_type))
return ExprError();
if (Context.getTypeInfoInChars(AtomTy).Width.isZero()) {
Diag(ExprRange.getBegin(), diag::err_atomic_builtin_must_be_pointer)
<< Ptr->getType() << 1 << Ptr->getSourceRange();
return ExprError();
if (Form != TestAndSet && Form != Clear) {
// Pointer to object of size zero is not allowed.
if (RequireCompleteType(Ptr->getBeginLoc(), AtomTy,
diag::err_incomplete_type))
return ExprError();

if (Context.getTypeInfoInChars(AtomTy).Width.isZero()) {
Diag(ExprRange.getBegin(), diag::err_atomic_builtin_must_be_pointer)
<< Ptr->getType() << 1 << Ptr->getSourceRange();
return ExprError();
}
} else {
// The __atomic_clear and __atomic_test_and_set intrinsics accept any
Copy link
Member

@jyknight jyknight Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this placement of the new code confusing, because we've already derived some values from the pointee-type.

I think it'd be clearer to check Form == TestAndSet && Form = Clear after line 3903 (before creating AtomTy/ValueType locals), and do the cast there -- assigning to both Ptr and pointerType. That way it's obvious that we are actually aren't using the original pointee-type for anything.

Copy link
Collaborator Author

@ostannard ostannard Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried that, but it results in the block above, which reports an error is a const pointer is used, always printing a char * type instead of the actual type from the source code. However I can update this to keep AtomTy and ValType up to date when the pointer type changes, so that the rest of the checks work correctly.

Copy link
Member

@jyknight jyknight Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, SGTM!

// non-const pointer type, including void* and pointers to incomplete
// structs, but only access the first byte.
if (AtomTy.isVolatileQualified())
Ptr = ImpCastExprToType(
Ptr,
Context.getPointerType(Context.getVolatileType(Context.CharTy)),
CK_BitCast)
.get();
else
Ptr = ImpCastExprToType(Ptr, Context.getPointerType(Context.CharTy),
CK_BitCast)
.get();
}

// For an arithmetic operation, the implied arithmetic must be well-formed.
Expand Down Expand Up @@ -3963,8 +3995,8 @@ ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
return ExprError();
}

if (!IsC11 && !AtomTy.isTriviallyCopyableType(Context) &&
!AtomTy->isScalarType()) {
if (!IsC11 && Form != TestAndSet && Form != Clear &&
!AtomTy.isTriviallyCopyableType(Context) && !AtomTy->isScalarType()) {
Copy link
Member

@jyknight jyknight Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then you can remove this diff, because the AtomTy will be (correctly) char.

Copy link
Collaborator Author

@ostannard ostannard Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

// For GNU atomics, require a trivially-copyable type. This is not part of
// the GNU atomics specification but we enforce it for consistency with
// other atomics which generally all require a trivially-copyable type. This
Expand Down Expand Up @@ -3997,10 +4029,10 @@ ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
ValType.removeLocalVolatile();
ValType.removeLocalConst();
QualType ResultType = ValType;
if (Form == Copy || Form == LoadCopy || Form == GNUXchg ||
Form == Init)
if (Form == Copy || Form == LoadCopy || Form == GNUXchg || Form == Init ||
Form == Clear)
ResultType = Context.VoidTy;
else if (Form == C11CmpXchg || Form == GNUCmpXchg)
else if (Form == C11CmpXchg || Form == GNUCmpXchg || Form == TestAndSet)
ResultType = Context.BoolTy;

// The type of a parameter passed 'by value'. In the GNU atomics, such
Expand Down Expand Up @@ -4045,6 +4077,10 @@ ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
APIOrderedArgs.push_back(Args[1]); // Order
APIOrderedArgs.push_back(Args[3]); // OrderFail
break;
case TestAndSet:
case Clear:
APIOrderedArgs.push_back(Args[1]); // Order
break;
}
} else
APIOrderedArgs.append(Args.begin(), Args.end());
Expand Down Expand Up @@ -4130,6 +4166,8 @@ ExprResult Sema::BuildAtomicExpr(SourceRange CallRange, SourceRange ExprRange,
SubExprs.push_back(APIOrderedArgs[1]); // Val1
break;
case Load:
case TestAndSet:
case Clear:
SubExprs.push_back(APIOrderedArgs[1]); // Order
break;
case LoadCopy:
Expand Down
Loading
Loading