[PATCH] D83340: Prohibit use of _ExtInt in atomic intrinsic

Tue Jul 14 12:00:06 PDT 2020

erichkeane added a comment.

In D83340#2149100 <https://reviews.llvm.org/D83340#2149100>, @jtmott-intel wrote:

> @jfb Ah, I feel I better understand your original question now. Thinking through how we would want `__atomic`s 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.

In D83340#2151198 <https://reviews.llvm.org/D83340#2151198>, @keryell wrote:

> 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83340/new/

https://reviews.llvm.org/D83340