CIRGen support for __sync_{and,or,xor,nand}_and_fetch builtin

[bcardosolopes]

add and sub are already supported.

[moar55]

I would like to take this issue.

[bcardosolopes]

@moar55 neat, assigned to you!

[moar55]

Hey, I was looking at the previous tests. And was wondering why does sync_op_and_fetch translates to

cir.atomic.fetch(add, {{%.*}} : !cir.ptr<!s8i>, [[VAL0]] : !s8i, seq_cst) fetch_first : !s8i
cir.binop(add, [[RES0]], [[VAL0]]) : !s8i

Wouldn't it be less instructions if fetch_first was not set (and thus the following binop wouldn't be needed.
Or is there a reason for this that I may not be aware of?

[moar55]

One other thing more related to this task, I don't see the BinOpKind::Nand. Should this be added along with the lowering logic to mlir::llvm?

[bcardosolopes]

Wouldn't it be less instructions if fetch_first was not set (and thus the following binop wouldn't be needed.
Or is there a reason for this that I may not be aware of?

What have you found when you compared to LLVM output from OG? We go about the same

[bcardosolopes]

One other thing more related to this task, I don't see the BinOpKind::Nand. Should this be added along with the lowering logic to mlir::llvm?

Do you need it as part of implementing anything? If so yes, if just for completeness best we don't.

[moar55]

One other thing more related to this task, I don't see the BinOpKind::Nand. Should this be added along with the lowering logic to mlir::llvm?

Do you need it as part of implementing anything? If so yes, if just for completeness best we don't.

Yes it seems so, because the allowed binary operations don't contain nand, and you need that for lowering __sync_nand_and_fetch

[moar55]

Should I maybe create that in its own commit first?

[bcardosolopes]

I suggest the opposite route: first create all variations that currently have binops, after that lands you go about introducing nand and the sync/fetch support. How does that sound?

[moar55]

Sounds perfectly fine :)

[moar55]

Actually I realized, that a Nand BinOpKind is not needed and quite unecessary since it doesn't match any real operator in C++. I found an implementation of sync_nand_and_fetch in clang/lib/CodeGen/CGBuiltin.cpp that makes sense to be imitated in the MLIR way.

[moar55]

I think this can be closed now.

[bcardosolopes]

Thanks @moar55

[moar55]

Sure! 😄