Skip to content
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

Merged
merged 11 commits into from
Mar 12, 2020

Conversation

Vexu
Copy link
Member

@Vexu Vexu commented Mar 10, 2020

  • Support bools
  • Support non power of two and smaller than byte sized integers
  • Update docs

Closes #1549
Closes #1220

@LemonBoy
Copy link
Contributor

I've briefly skimmed over the codegen changes but I feel the widen+truncate strategy only works for stand-alone i1 that LLVM legalizes as i8, for example an i1 in a packed structure invalidates that assumption.

@andrewrk
Copy link
Member

andrewrk commented Mar 10, 2020

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 *i1 because @alignOf(i1) == 1 which makes it the same thing as i8. We should be able to safely pointer cast to power-of-two integers with the same alignment.

@Vexu Vexu marked this pull request as ready for review March 11, 2020 11:46
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) {
Copy link
Contributor

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);
Copy link
Contributor

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?

Copy link
Member Author

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) {
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member

@andrewrk andrewrk left a 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.

@BarabasGitHub
Copy link
Contributor

However it should work fine for *i1 because @Alignof(i1) == 1 which makes it the same thing as i8.

Is that true? So, in a bit easier case, if you have an u8 align(2) and a normal u8 and you put in a struct like this:

struct {
    a: u8 align(2),
    b: u8,
};

Then the size will be 3 (or rather 4) bytes? I would have expect it to be 2 bytes, because you can put b right behind a, without violating the alignment requirement for a.

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)

packed struct {
    a: u1 align(1),
    b: u1,
};

@andrewrk
Copy link
Member

Then the size will be 3 (or rather 4) bytes?

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));
}
| *"A", *"size", 2, *"align", 1
| *"B", *"size", 2, *"align", 2

@BarabasGitHub
Copy link
Contributor

Is it then safe to do atomic operations on a in this struct? That was basically my question.

const B = packed struct {
    a: u1 align(1),
    b: u1,
};
| *"B", *"size", 1, *"align", 1

And if it's safe for a, why not for b?

@andrewrk
Copy link
Member

#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 *T.

*align(1) u1 == *u1 so that works fine. If you look at the pointer type of the b field it's something like *align(0:1:1) u1, which does not type coerce to *u1, so that would be a compile error.

@BarabasGitHub
Copy link
Contributor

Alright, thanks for answering my questions :)

@andrewrk andrewrk merged commit f51bec3 into ziglang:master Mar 12, 2020
@Vexu Vexu deleted the small-atomics branch March 12, 2020 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@atomicRmw on u0 @atomicRmw - support bool and non-power-of-2 int types
4 participants