Page MenuHomePhabricator

Prohibit use of _ExtInt in atomic intrinsic
ClosedPublic

Authored by jtmott-intel on Jul 7 2020, 1:07 PM.

Details

Summary

The _ExtInt type allows custom width integers, but the atomic memory access's operand must have a power-of-two size. _ExtInts with non-power-of-two size should not be allowed for atomic intrinsic.

Before this change:

$ cat test.c

typedef unsigned _ExtInt(42) dtype;
void verify_binary_op_nand(dtype* pval1, dtype val2)
{    __sync_nand_and_fetch(pval1, val2); }

$ clang test.c

clang-11: /home/ubuntu/llvm_workspace/llvm/clang/lib/CodeGen/CGBuiltin.cpp:117: llvm::Value* 
EmitToInt(clang::CodeGen::CodeGenFunction&, llvm::Value*, clang::QualType, llvm::IntegerType*): Assertion `V->getType() == IntType' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.

After this change:

$ clang test.c

test.c:3:30: error: Atomic memory operand must have a power-of-two size
{    __sync_nand_and_fetch(pval1, val2); }
                       ^

List of the atomic intrinsics that have this problem:

  • __sync_fetch_and_add
  • __sync_fetch_and_sub
  • __sync_fetch_and_or
  • __sync_fetch_and_and
  • __sync_fetch_and_xor
  • __sync_fetch_and_nand
  • __sync_nand_and_fetch
  • __sync_and_and_fetch
  • __sync_add_and_fetch
  • __sync_sub_and_fetch
  • __sync_or_and_fetch
  • __sync_xor_and_fetch
  • __sync_fetch_and_min
  • __sync_fetch_and_max
  • __sync_fetch_and_umin
  • __sync_fetch_and_umax
  • __sync_val_compare_and_swap
  • __sync_bool_compare_and_swap

Diff Detail

Event Timeline

jtmott-intel created this revision.Jul 7 2020, 1:07 PM
jfb added a comment.Jul 7 2020, 1:12 PM

Are the _Extint uses here as you'd expect too? (using _Atomic): https://reviews.llvm.org/D79279
We have other atomic primitives, such as _atomic_, do these need updates too?

erichkeane added inline comments.Jul 7 2020, 1:28 PM
clang/lib/Sema/SemaChecking.cpp
5355

const auto *.

5357

use llvm::isPowerOf2_64

@jfb The __atomic builtins seem to round up to the nearest power of 2, and they appear to work because of that. Is there a specific use case or reproducer you were concerned about?

jtmott-intel added inline comments.Jul 13 2020, 11:51 AM
clang/lib/Sema/SemaChecking.cpp
5355

The pointer seems to be included in the deduced auto type. Unless there's a style convention that we'd rather write the * ?

erichkeane added inline comments.Jul 13 2020, 12:02 PM
clang/lib/Sema/SemaChecking.cpp
5355

Yes, the llvm coding guidelines for 'auto' require you use the pointer when its a pointer.

Addressed review comments.

jtmott-intel marked 2 inline comments as done.Jul 13 2020, 12:39 PM

What about powers-of-2 larger than, say, 256 bit? So a 512 bit value. Does this give reasonable codegen?

@erichkeane Assuming we're back to talking about the __sync functions, I get the following diagnostic.

test.c:22:6: error: address argument to atomic builtin must be a pointer to 1,2,4,8 or 16 byte type ('dtype *' (aka 'unsigned _ExtInt(512) *') invalid)
{    __sync_nand_and_fetch(pval1, val2); }
     ^                     ~~~~~
1 error generated.

Is this what you're referring to? (In highsight I probably could have piggybacked on this diagnostic for the _ExtInt check.)

erichkeane accepted this revision.Jul 13 2020, 5:17 PM

@erichkeane Assuming we're back to talking about the __sync functions, I get the following diagnostic.

test.c:22:6: error: address argument to atomic builtin must be a pointer to 1,2,4,8 or 16 byte type ('dtype *' (aka 'unsigned _ExtInt(512) *') invalid)
{    __sync_nand_and_fetch(pval1, val2); }
     ^                     ~~~~~
1 error generated.

Is this what you're referring to? (In highsight I probably could have piggybacked on this diagnostic for the _ExtInt check.)

Yep, thats what I was worried about, thanks!

This revision is now accepted and ready to land.Jul 13 2020, 5:17 PM

I don't have commit access. For whoever performs the commit, here's a (draft) commit message:

[clang] Prohibit use of non-power-of-2 _ExtInt with __sync builtins
jfb added a comment.Jul 13 2020, 7:56 PM

@jfb The __atomic builtins seem to round up to the nearest power of 2, and they appear to work because of that. Is there a specific use case or reproducer you were concerned about?

This is ABI, so I don’t want to ship “whatever happens to be implemented”. It ought to be well thought out. Specifically for atomic, GCC and LLVM diverge on those details, and I wouldn’t want this new feature to continue the divergence.

jtmott-intel added a comment.EditedJul 13 2020, 8:56 PM

@jfb Ah, I feel I better understand your original question now. Thinking through how we would want __atomics to behave is great, and I'm genuinely glad someone's championing it. It also sounds a *smidgen* beyond the scope of what I was trying to accomplish with this particular patch. :-) This patch doesn't alter the behavior of the __atomic builtins one way or the other. It alters the behavior of the __sync builtins only to change a debug assert failure into a diagnostic error. I believe the _ExtInt authors are tracking new bugs related to that feature. A new bug report could keep the discussion going and keep the issue visible.

@jfb Ah, I feel I better understand your original question now. Thinking through how we would want __atomics to behave is great, and I'm genuinely glad someone's championing it. It also sounds a *smidgen* beyond the scope of what I was trying to accomplish with this particular patch. :-) This patch doesn't alter the behavior of the __atomic builtins one way or the other. It alters the behavior of the __sync builtins only to change a debug assert failure into a diagnostic error. I believe the _ExtInt authors are tracking new bugs related to that feature. A new bug report could keep the discussion going and keep the issue visible.

Jeff: Please submit a patch to at least disable ExtInt for these atomic builtins, that way we can design their behavior later. I agree that this shouldn't block this patch, but we need ASAP.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2020, 6:11 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

The idea of this _ExtInt is to have some extensions.
Since it is an extension, why preventing its use?
For example if I want my 18 bit FPGA BRAM to be accessed atomically?
Or is there an assumption that atomic access can be enabled back with some other mode, such as SYCL or HLS C++?

@jfb Ah, I feel I better understand your original question now. Thinking through how we would want __atomics to behave is great, and I'm genuinely glad someone's championing it. It also sounds a *smidgen* beyond the scope of what I was trying to accomplish with this particular patch. :-) This patch doesn't alter the behavior of the __atomic builtins one way or the other. It alters the behavior of the __sync builtins only to change a debug assert failure into a diagnostic error. I believe the _ExtInt authors are tracking new bugs related to that feature. A new bug report could keep the discussion going and keep the issue visible.

Jeff: Please submit a patch to at least disable ExtInt for these atomic builtins, that way we can design their behavior later. I agree that this shouldn't block this patch, but we need it soon.

The idea of this _ExtInt is to have some extensions.
Since it is an extension, why preventing its use?
For example if I want my 18 bit FPGA BRAM to be accessed atomically?
Or is there an assumption that atomic access can be enabled back with some other mode, such as SYCL or HLS C++?

The idea is that until we do an audit/design for the way atomic builtins are supposed to work, we should leave them restricted as they can bind us to a sub-optimal implementation via ABI. If you have a platform/sub language that spends a good amount of time designing/defining the mechanism in an informed way(rather than organically inheriting them), enabling it for that situation is a perfectly reasonable thing to do.

In the meantime, we would like to disable things that aren't perfectly sensible in order to preserve the ability to do the above.