Skip to content

Made load a new format callable #2106

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Made load a new format callable #2106

wants to merge 13 commits into from

Conversation

SadiinsoSnowfall
Copy link
Collaborator

No description provided.

@SadiinsoSnowfall SadiinsoSnowfall force-pushed the callable/load branch 4 times, most recently from a5ffc3c to c71d6fc Compare June 4, 2025 09:13
Copy link
Collaborator

@DenisYaroshevskiy DenisYaroshevskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

algo changes look OK. THe load itself - didn't look yet.

@SadiinsoSnowfall SadiinsoSnowfall marked this pull request as ready for review June 4, 2025 10:45
@SadiinsoSnowfall
Copy link
Collaborator Author

Notes:
I moved everything to detail/function. A part of the x86 and generic impls previously were in core/regular.

There's now the following functions :

  • load_ (iterator flavour): handles the iterators directly, special case for logical values with !is_wide_logical ABIs.
  • load_ (normal): entry point for the callable impl, handles things that should always be done first: unsafe and algorithm-based loads, as well as handling bundle values. Once we have a safe non-bundle non-alg load, load_common is called.
  • load_common: the "main" function which handles the following cases:
    • if we are in aggregated or emulated ABI:
      • if there is no condition (ignore_none): call aggregate_load or piecewise_load
      • if there is: call load_cx_
    • if there is a backend that can handle the operation with a condition directly, call it (load_impl(api, cx, src, tgt))
    • if there is no condition, call the non-conditional form of the backend (load_impl(api, src, tgt)). Note: this must work or else it would mean that no load backends are defined for the current architecture.
    • if there is, call load_cx_
  • load_cx_: handle the conditions when not handled by the architecture-specific backends. Special-cased for logical values with !is_wide_logical ABIs.

load_common takes an api parameter so that architecture-specific backends are able to "bail out" and, for example, ask for load_cx_ to handle specific forms of conditional load.

A generic load_impl for logical values is provided. This is because most backends either do not handle logical loads or handle them in a similar manner. This function is special-cased for values with !is_wide_logical ABIs.


template<> struct supports_optimized_conversion<tag::load_> : std::true_type {};
template<detail::data_source Ptr, std::ptrdiff_t N>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concepts using detail is ofc not awesome, but this is not for you.

//================================================================================================
//! @addtogroup simd
//! @{
//! @var load
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jfalcou - docs in detail. Will they be rendered?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to move it to the non detail place like for bit_cast.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -6,95 +6,4 @@
//==================================================================================================
#pragma once

#include <eve/arch.hpp>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe actual load should be in this file instead of detail.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is this way cause of our adage of having the basics of wide, etc, not depending on the module/core. When everything settles on this front, we can revisit that. Also, the doc should be here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

}
else
{
return piecewise_load(src, tgt);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my sanity - where is the assert that there is enough space?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean a check to ensure that every element in the [b, e) range fits in the logical register ? There was previously no checks for that. I also suspect that the second iterator was only used to differentiate the implementation for the standard one as it has never been used, or so it seems.

{
return load(ignore_none, safe, as<Wide>{}, b, e);
if constexpr (logical_simd_value<Wide> && !Wide::abi_type::is_wide_logical)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some confusion here.

This code is duplicated between this overload and the next overload.
Should be unified for sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, the iterator path always end up calling piecewise_load whereas the logical path could have arch-specific backends.

return aggregate_load(tgt,ptr);
if constexpr (!std::is_pointer_v<DS>)
{
constexpr bool is_aligned_enough = c_t() * sizeof(e_t) >= DS::alignment();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm so confused to what;'s happening.

DS is data_source. We use that in the API of load. Here we call ::alignmetn on it. We obv can't do that for pointers. What's happening.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well there's a !std::is_pointer_v<DS> check right before the call to alignment

@SadiinsoSnowfall SadiinsoSnowfall force-pushed the callable/load branch 2 times, most recently from fe19266 to 975a766 Compare June 16, 2025 11:12
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

Successfully merging this pull request may close these issues.

3 participants