[SYCL][USM] Improve USM Allocator.

[jbrodman]

Add ability to use std::allocate_shared.
Add equality operators for allocators.
Add tests.

Disallow device allocations in usm_allocator as there are too many incompatibilities with how C++ allocators are used.

[sergey-semenov]

Other than a few comments, the changes LGTM. Please, take a look at the failing allocator_shared test.

[sergey-semenov]

@jbrodman Please, take a look at the unexpected pass of allocator_equal with CUDA

[rolandschulz]

How could an implementation do the right thing? The value_type isn't passed to aligned_alloc and therefore the implementation doesn't know about the required alignment. Maybe the best solution would be if line 29 would be changed to size_t Alignment = alignof(T).

[jbrodman]

0 is treated as "default - do something legal"

[rolandschulz]

Yes the template argument is fine as-is. What's not OK is that you only pass getAlignment to aligned_alloc and it might be zero. Alignment is 0 and T is over-aligned there is no way for aligned_alloc to know.

You can either

• Change the template argument
• Uncomment the next few lines
• Or pass T to aligned_alloc

[rolandschulz]

Yes the template argument is fine as-is. What's not OK is that you only pass getAlignment to aligned_alloc and it might be zero. Alignment is 0 and T is over-aligned there is no way for aligned_alloc to know.

You can either

• Change the template argument
• Uncomment the next few lines
• Or pass T to aligned_alloc

[jbrodman]

0 is valid for alignment - "just do the default thing" - at the end of the day these get turned into byte amt mallocs - all type info is discarded.

Aligned_alloc( align = 0) is equivalent to just calling malloc.

[rolandschulz]

0 is valid for alignment - "just do the default thing" - at the end of the day these get turned into byte amt mallocs - all type info is discarded.

Aligned_alloc( align = 0) is equivalent to just calling malloc.

My point is we shouldn't do that. By default it should have the same alignment as std::alloc or new. They both use the alignment required by the type as the default alignment. That way a user can use this allocator for an over-aligned type and the default correctly works.

[jbrodman]

0 is valid for alignment - "just do the default thing" - at the end of the day these get turned into byte amt mallocs - all type info is discarded.
Aligned_alloc( align = 0) is equivalent to just calling malloc.

My point is we shouldn't do that. By default it should have the same alignment as std::alloc or new. They both use the alignment required by the type as the default alignment. That way a user can use this allocator for an over-aligned type and the default correctly works.

So what's the change?
Change Alignment = 0 to Alignment = alignof(T)?

[rolandschulz]

So what's the change?
Change Alignment = 0 to Alignment = alignof(T)?

Options:

1. Alignment = 0 to Alignment = alignof(T) for default template argument. I think this requires also a change to rebind: typedef usm_allocator<U, AllocKind, max(Alignment,alignof(U))> other;
2. Change getAlignment to return ((Alignment == 0)? alignof(value_type) : Alignment);

Not sure which option is better.

[romanovvlad]

@jbrodman
With this patch, what happens if a user tries to use usm_allocator which allocates device type of memory for containers like std::vector? Could you please check that the following works or gives some diagnostic:

  cl::sycl::queue Q{};
  using USMAllocator= cl::sycl::usm_allocator<int, cl::sycl::usm::alloc::device>;
  USMAllocator Allocator(Q.get_context(), Q.get_device());
  std::vector<int, USMAllocator> Vec1(/*Size=*/1, Allocator);
  std::vector<int, USMAllocator> Vec2(/*Size=*/1, /*InitVal=*/42, Allocator);
  std::vector<int, USMAllocator> Vec3 = Vec1;
  Vec.resize(43);

[jbrodman]

There are too many problems with device allocations. Too many C++ allocator-isms just don't work, so there's a static_assert that fires at compile time if you try to use them. We have to disallow them with the allocator interface.

[romanovvlad]

There are too many problems with device allocations. Too many C++ allocator-isms just don't work, so there's a static_assert that fires at compile time if you try to use them. We have to disallow them with the allocator interface.

I see.

[romanovvlad]

@jbrodman Could you please provide a final commit message? The text in the first comment looks outdated.

[bader]

One more question: what is the plan for PR #1577 once this PR is merged?
Tagging @fwyzard.

[fwyzard]

I hope to be able to come back to work on the CUDA support during the summer - i.e. this or next week...

[jbrodman]

@jbrodman Could you please provide a final commit message? The text in the first comment looks outdated.

@romanovvlad what's the best Github way to do that?

[bader]

@jbrodman Could you please provide a final commit message? The text in the first comment looks outdated.

@romanovvlad what's the best Github way to do that?

Can I use current PR description as a commit message?

Add ability to use std::allocate_shared.
Add equality operators for allocators.
Add tests.

Disallow device allocations in usm_allocator as there are too many incompatibilities with how C++ allocators are used.

Commit title: [SYCL][USM] Improve USM Allocator.

[jbrodman]

@jbrodman Could you please provide a final commit message? The text in the first comment looks outdated.

@romanovvlad what's the best Github way to do that?

Can I use current PR description as a commit message?

Add ability to use std::allocate_shared.
Add equality operators for allocators.
Add tests.
Disallow device allocations in usm_allocator as there are too many incompatibilities with how C++ allocators are used.

Commit title: [SYCL][USM] Improve USM Allocator.

Sure?