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

_BitInt(N) with large N #1212

Open
dkolsen-pgi opened this issue Dec 5, 2024 · 6 comments
Open

_BitInt(N) with large N #1212

dkolsen-pgi opened this issue Dec 5, 2024 · 6 comments

Comments

@dkolsen-pgi
Copy link
Collaborator

ClangIR implements the C23 _BitInt(N) types as cir.int. That works fine for N <= 128, which is currently the upper limit for the width of cir.int. But Clang supports _BitInt(N) for much larger N, up to 8,388,608. ClangIR should also support large _BitInt types. I don't know whether the right way to do this is to extend the upper limit of cir.int or to use a different CIR type for large _BitInt types.

$ cat bitint.cpp
int main() {
  _BitInt(130) a = 42;
  _BitInt(130) b = 100;
  _BitInt(130) c = a + b;
  return c;
}
$ clang++ bitint.cpp
$ clang++ -fclangir bitint.cpp
error: IntType only supports widths from 1 up to 128
clang-19: /local/home/dolsen/clangir/llvm/../mlir/include/mlir/IR/StorageUniquerSupport.h:179: static ConcreteT mlir::detail::StorageUserBase<ConcreteT, BaseT, StorageT, UniquerT, Traits>::get(mlir::MLIRContext*, Args&& ...) [with Args = {unsigned int, bool}; ConcreteT = cir::IntType; BaseT = mlir::Type; StorageT = cir::detail::IntTypeStorage; UniquerT = mlir::detail::TypeUniquer; Traits = {mlir::DataLayoutTypeInterface::Trait}]: Assertion `succeeded( ConcreteT::verifyInvariants(getDefaultDiagnosticEmitFn(ctx), args...))' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
...
@keryell
Copy link
Collaborator

keryell commented Dec 5, 2024

This is useful for FPGA where we love 512 bit busses and transfer explicit 512 bit data chunks.

It looks to me that using the same cir.int simplifies IR and code gen.
@tahonermann @aisoard How is the data layout problem solved in SYCL for FPGA and HLS C++? 🤔

@tahonermann
Copy link
Contributor

@keryell, the data layout problem isn't solved for SYCL. _BitInt types don't exist in C++ and, even if they did, they probably wouldn't be included in the set of types permitted in device code (see section 5.5, "Built-in scalar data types", of the SYCL 2020 specification). ABI issues would have to be worked out to support them and that is already kind of a messy issue in SYCL 2020; see this issue regarding pointer-to-member types for example.

@aisoard
Copy link
Contributor

aisoard commented Dec 12, 2024

@keryell for AMD/Xilinx FPGA the data-layout is "little endian, padded and aligned to the 'next' power of two", it helps a lot with vectorization and imposing a strict layout makes everybody's life easier. We made _BitInt "exists" in C++ when the target is FPGA and adding the alignas enforced the alignment and padding.

On the CPU side of things we were just using a struct containing an array of uint64_t with the alignas attribute (if they are 64 or bigger, else it's using the appropriate uintN_t). It's cumbersome, but _BitInt isn't available and std::bitset don't provide integer operations, so we didn't have much choice.

@aisoard
Copy link
Contributor

aisoard commented Dec 12, 2024

Also, we are glad that Clang upped the _BitInt limit because we have (crazy) users playing around with 8192 bit busses...

@keryell
Copy link
Collaborator

keryell commented Dec 17, 2024

@keryell, the data layout problem isn't solved for SYCL. _BitInt types don't exist in C++ and, even if they did, they probably wouldn't be included in the set of types permitted in device code (see section 5.5, "Built-in scalar data types", of the SYCL 2020 specification). ABI issues would have to be worked out to support them and that is already kind of a messy issue in SYCL 2020; see this issue regarding pointer-to-member types for example.

@tahonermann You are mentioning a more general problem on how to lower weird things like function pointers in a compatible way between host and device, which is a mess if the device has no function pointers at the first place, for example.
But this is more complex than what I was thinking of, like device data structures having _BitInt, without any interoperability with the host. I think this is what Intel SYCL for FPGA supports today. Perhaps they use the same padding and alignment to the next power of 2 like AMD does in Vitis HLS C++?
It looks like aligning in memory to the "next inclusive power of 2" is a safe ABI, even if it is sub-optimal on some targets with a more complex memory system allowing lower alignment without speed degradation.

@tahonermann
Copy link
Contributor

@keryell, no, I wasn't referring to function pointers; I was referring to pointer-to-member types which (presumably) do exist in device code and can be used as the type of data members (the SYCL 2020 specification doesn't acknowledge the existence of such types, so they are neither excluded as function pointers are nor are they explicitly allowed). The concern is not with lowering, but with object representation and whether such types are device copyable; the same concerns as for _BitInt types.

The SYCL specification can't dictate ABI matters and therefore can't require _BitInt types to have a compatible object representation across the host and each device, nor can it require them to be device copyable. At best, it can specify them as optionally available when the host and device ABI object representations match. Regardless of what the SYCL specification says, implementations can always offer support for _BitInt when the ABI object representations are compatible across the host and each device.

A quick glance at the Clang supported targets that currently have _BitInt support enabled indicates they are all using the same power of two size and alignment at present. They do differ with regard to their maximum alignment and size though.

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

No branches or pull requests

4 participants