-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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.
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
Support atomic operations with bools and non power of two integers #4707
Conversation
I've briefly skimmed over the codegen changes but I feel the widen+truncate strategy only works for stand-alone |
In zig the atomic builtins take a pointer, which has alignment metadata in the pointer type. If it's a pointer to e.g. an i1 from a packed struct, then the pointer type alignment will have sub-byte metadata. That will be a compile error, as it cannot be atomic. However it should work fine for |
ZigType *result_type = get_optional_type(ira->codegen, operand_type); | ||
|
||
// special case zero bit types | ||
if (type_has_one_possible_value(ira->codegen, operand_type) == OnePossibleValueYes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're only interested in the type size use type_has_bits2
instead
int_type->data.integral.bit_count)); | ||
return ira->codegen->builtin_types.entry_invalid; | ||
} | ||
uint32_t max_atomic_bits = target_arch_largest_atomic_bits(ira->codegen->zig_target->arch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check against target_arch_largest_atomic_bits
is gone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I combined the int check and enum tag type check so it is now a few lines above.
@@ -5219,11 +5219,50 @@ static enum ZigLLVM_AtomicRMWBinOp to_ZigLLVMAtomicRMWBinOp(AtomicRmwOp op, bool | |||
zig_unreachable(); | |||
} | |||
|
|||
static LLVMTypeRef get_atomic_abi_type(CodeGen *g, IrInstGen *instruction) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an ad-hoc version of LLVMABISizeOfType
.
One more idea, since you're already converting values forth and back you may also want to tackle #4457 in a general way by simply bitcasting floats back to an appropriately-sized iN
type to gently sidestep this limitation in some backends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you may also want to tackle #4457 in a general way
That would work for .Xchg
but we also support .Add
and .Sub
for floats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh lord, I assumed only Xchg
was valid for floats
. Carry on then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work, looking forward to merging this.
Is that true? So, in a bit easier case, if you have an
Then the size will be 3 (or rather 4) bytes? I would have expect it to be 2 bytes, because you can put So my question is basically, what does it do here? Is the size of this struct 2 or 1 bytes? (Currently it's 1, although I'm not sure it works as intended)
|
You can test these things: const A = struct {
a: u8,
b: u8,
};
const B = struct {
a: u8 align(2),
b: u8,
};
comptime {
@compileLog("A", "size", @sizeOf(A), "align", @alignOf(A));
@compileLog("B", "size", @sizeOf(B), "align", @alignOf(B));
}
|
Is it then safe to do atomic operations on
And if it's safe for |
#3133 and related bugs have been postponed to after 0.6.0, so there may be bugs with offsets or alignment of packed struct fields for now. But the answer to your question is that the atomic operations coerce operands to
|
Alright, thanks for answering my questions :) |
Closes #1549
Closes #1220