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.

Sign up for GitHub

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

Jump to bottom

Evaluate tensorstoreToITKComponentType at compile-time, using constexpr #68

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Evaluate tensorstoreToITKComponentType at compile-time, using constexpr #68

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
63c4d91
STYLE: Declare component type conversion functions constexpr
N-Dekker Aug 20, 2024
6678cc4
PERF: Call tensorstoreToITKComponentType at compile-time, by constexpr
N-Dekker Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 55 additions & 52 deletions src/itkOMEZarrNGFFImageIO.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ OMEZarrNGFFImageIO::PrintSelf(std::ostream & os, Indent indent) const
os << indent << "ChannelIndex: " << m_ChannelIndex << std::endl;
}

IOComponentEnum
constexpr IOComponentEnum
tensorstoreToITKComponentType(const tensorstore::DataType dtype)
{
switch (dtype.id())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting compile errors from Ubuntu/GCC, https://open.cdash.org/viewBuildError.php?buildid=9845800 at this particular dtype.id() call:

itkOMEZarrNGFFImageIO.cxx: In instantiation of 'constexpr const IOComponentEnum itk::toITKComponentType<float>':
itkOMEZarrNGFFImageIO.cxx:643:3:   required from here
itkOMEZarrNGFFImageIO.cxx:251:84:   in 'constexpr' expansion of 'itk::tensorstoreToITKComponentType(tensorstore::dtype_v<float>.tensorstore::StaticDataType<float>::operator tensorstore::DataType())'
itkOMEZarrNGFFImageIO.cxx:141:19:   in 'constexpr' expansion of 'dtype.tensorstore::DataType::id()'
itkOMEZarrNGFFImageIO.cxx:251:34: error: the value of 'tensorstore::internal_data_type::MakeDataTypeOperations<float>::operations' is not usable in a constant expression

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just fixed, by google/tensorstore@91ea2a2 😃

Copy link
Member

Choose a reason for hiding this comment

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

Let's get #72 and #73 in before we try updating version of tensorstore we use. Or do you want to try that right away Niels?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's get #72 and #73 in before we try updating version of tensorstore we use.

Sounds like a plan 👍

Or do you want to try that right away Niels?

No, this one (#68) can wait a little longer, no problem!

Expand Down Expand Up @@ -177,7 +177,7 @@ tensorstoreToITKComponentType(const tensorstore::DataType dtype)
}
}

tensorstore::DataType
constexpr tensorstore::DataType
itkToTensorstoreComponentType(const IOComponentEnum itkComponentType)
{
switch (itkComponentType)
Expand Down Expand Up @@ -246,6 +246,10 @@ itkToTensorstoreComponentType(const IOComponentEnum itkComponentType)
}
}

// Variable template, ensuring that tensorstoreToITKComponentType is evaluated at compile-time.
template <typename T>
static constexpr IOComponentEnum toITKComponentType = tensorstoreToITKComponentType(tensorstore::dtype_v<T>);
Comment on lines +250 to +251
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could bypass the troublesome compile-time evaluation of tensorstoreToITKComponentType/tensorstore::DataType::id() by defining the variable template as follows:

template <typename T>
static constexpr IOComponentEnum toITKComponentType =
  std::is_same_v<T, int8_t> ? IOComponentEnum::CHAR :
  std::is_same_v<T, uint8_t> ? IOComponentEnum::UCHAR :
  std::is_same_v<T, int16_t> ? IOComponentEnum::SHORT :
  std::is_same_v<T, uint16_t> ? IOComponentEnum::USHORT :
  std::is_same_v<T, int32_t> ? IOComponentEnum::INT :
  std::is_same_v<T, uint32_t> ? IOComponentEnum::UINT :
  std::is_same_v<T, int64_t> ? IOComponentEnum::LONGLONG :
  std::is_same_v<T, uint64_t> ? IOComponentEnum::ULONGLONG :
  std::is_same_v<T, float> ? IOComponentEnum::FLOAT :
  std::is_same_v<T, double> ? IOComponentEnum::DOUBLE :
  IOComponentEnum::UNKNOWNCOMPONENTTYPE;

Do we like that? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

It is probably better if they fix it. Let's wait a few days?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, let's wait a few days 👍 By the way, clang-format would make the "is_same_v based" definition from #68 (comment) look like this:

template <typename T>
static constexpr IOComponentEnum toITKComponentType =
  std::is_same_v<T, int8_t>
    ? IOComponentEnum::CHAR
    : std::is_same_v<T, uint8_t>
        ? IOComponentEnum::UCHAR
        : std::is_same_v<T, int16_t>
            ? IOComponentEnum::SHORT
            : std::is_same_v<T, uint16_t>
                ? IOComponentEnum::USHORT
                : std::is_same_v<T, int32_t>
                    ? IOComponentEnum::INT
                    : std::is_same_v<T, uint32_t>
                        ? IOComponentEnum::UINT
                        : std::is_same_v<T, int64_t>
                            ? IOComponentEnum::LONGLONG
                            : std::is_same_v<T, uint64_t>
                                ? IOComponentEnum::ULONGLONG
                                : std::is_same_v<T, float>
                                    ? IOComponentEnum::FLOAT
                                    : std::is_same_v<T, double> ? IOComponentEnum::DOUBLE
                                                                : IOComponentEnum::UNKNOWNCOMPONENTTYPE;

Wonderful, isn't is? 😸


// Returns TensorStore KvStore driver name appropriate for this path.
// Options are file, zip. TODO: http, gcs (GoogleCouldStorage), etc.
std::string
Expand Down Expand Up @@ -602,11 +606,10 @@ OMEZarrNGFFImageIO::ReadImageInformation()
}

// We call tensorstoreToITKComponentType for each type.
// Hopefully compiler will optimize it away via constant propagation and inlining.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This PR proposes to remove the comment saying that "hopefully compiler will optimize it away", as the compiler will optimize it away now, with this commit! If I understand correctly!

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps leave the comment, just change it? "Compiler should optimize away all unused branches"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, @dzenanz but I'm not entirely sure. It now (originally) says:

We call tensorstoreToITKComponentType for each type. Hopefully compiler will optimize it away [...]

In this case, "it" refers to the tensorstoreToITKComponentType calls, right? Those are certainly optimized away, with this PR.

Which unused branches do you refer to?

Copy link
Member

Choose a reason for hiding this comment

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

tensorstoreToITKComponentType calls have many if-else branches.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean that tensorstoreToITKComponentType has many case's? As in:

ITKIOOMEZarrNGFF/src/itkOMEZarrNGFFImageIO.cxx

Lines 139 to 178 in 98a43d0

tensorstoreToITKComponentType(const tensorstore::DataType dtype)
{
switch (dtype.id())
{
case tensorstore::DataTypeId::char_t:
case tensorstore::DataTypeId::int8_t:
return IOComponentEnum::CHAR;
case tensorstore::DataTypeId::byte_t:
case tensorstore::DataTypeId::uint8_t:
return IOComponentEnum::UCHAR;
case tensorstore::DataTypeId::int16_t:
return IOComponentEnum::SHORT;
case tensorstore::DataTypeId::uint16_t:
return IOComponentEnum::USHORT;
case tensorstore::DataTypeId::int32_t:
return IOComponentEnum::INT;
case tensorstore::DataTypeId::uint32_t:
return IOComponentEnum::UINT;
case tensorstore::DataTypeId::int64_t:
return IOComponentEnum::LONGLONG;
case tensorstore::DataTypeId::uint64_t:
return IOComponentEnum::ULONGLONG;
case tensorstore::DataTypeId::float32_t:
return IOComponentEnum::FLOAT;
case tensorstore::DataTypeId::float64_t:
return IOComponentEnum::DOUBLE;
default:
return IOComponentEnum::UNKNOWNCOMPONENTTYPE;
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Both that and

ITKIOOMEZarrNGFF/src/itkOMEZarrNGFFImageIO.cxx

Lines 641 to 650 in 98a43d0

READ_ELEMENT_IF(int8_t)
READ_ELEMENT_IF(uint8_t)
READ_ELEMENT_IF(int16_t)
READ_ELEMENT_IF(uint16_t)
READ_ELEMENT_IF(int32_t)
READ_ELEMENT_IF(uint32_t)
READ_ELEMENT_IF(int64_t)
READ_ELEMENT_IF(uint64_t)
READ_ELEMENT_IF(float)
READ_ELEMENT_IF(double)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#define READ_ELEMENT_IF(typeName) \
else if (tensorstoreToITKComponentType(tensorstore::dtype_v<typeName>) == this->GetComponentType()) \
{ \
ReadFromStore<typeName>(store, storeIORegion, reinterpret_cast<typeName *>(buffer)); \
#define READ_ELEMENT_IF(typeName) \
else if (toITKComponentType<typeName> == this->GetComponentType()) \
{ \
ReadFromStore<typeName>(store, storeIORegion, reinterpret_cast<typeName *>(buffer)); \
}

void
Expand Down Expand Up @@ -708,51 +711,51 @@ OMEZarrNGFFImageIO::WriteImageInformation()


// We need to specify dtype for opening. As dtype is dependent on component type, this macro is long.
#define ELEMENT_WRITE(typeName) \
else if (tensorstoreToITKComponentType(tensorstore::dtype_v<typeName>) == this->GetComponentType()) \
{ \
if (sizeof(typeName) == 1) \
{ \
dtype = "|"; \
} \
if (std::numeric_limits<typeName>::is_integer) \
{ \
if (std::numeric_limits<typeName>::is_signed) \
{ \
dtype += 'i'; \
} \
else \
{ \
dtype += 'u'; \
} \
} \
else \
{ \
dtype += 'f'; \
} \
dtype += std::to_string(sizeof(typeName)); \
\
auto openFuture = tensorstore::Open( \
{ \
{ "driver", "zarr" }, \
{ "kvstore", { { "driver", driver }, { "path", this->m_FileName + "/" + path } } }, \
{ "metadata", \
{ \
{ "compressor", { { "id", "blosc" } } }, \
{ "dtype", dtype }, \
{ "shape", shape }, \
} }, \
}, \
tsContext, \
tensorstore::OpenMode::create | tensorstore::OpenMode::delete_existing, \
tensorstore::ReadWriteMode::read_write); \
TS_EVAL_CHECK(openFuture); \
\
auto writeStore = openFuture.value(); \
auto * p = reinterpret_cast<typeName const *>(buffer); \
auto arr = tensorstore::Array(p, shape, tensorstore::c_order); \
auto writeFuture = tensorstore::Write(tensorstore::UnownedToShared(arr), writeStore); \
TS_EVAL_CHECK(writeFuture); \
#define ELEMENT_WRITE(typeName) \
else if (toITKComponentType<typeName> == this->GetComponentType()) \
{ \
if (sizeof(typeName) == 1) \
{ \
dtype = "|"; \
} \
if (std::numeric_limits<typeName>::is_integer) \
{ \
if (std::numeric_limits<typeName>::is_signed) \
{ \
dtype += 'i'; \
} \
else \
{ \
dtype += 'u'; \
} \
} \
else \
{ \
dtype += 'f'; \
} \
dtype += std::to_string(sizeof(typeName)); \
\
auto openFuture = tensorstore::Open( \
{ \
{ "driver", "zarr" }, \
{ "kvstore", { { "driver", driver }, { "path", this->m_FileName + "/" + path } } }, \
{ "metadata", \
{ \
{ "compressor", { { "id", "blosc" } } }, \
{ "dtype", dtype }, \
{ "shape", shape }, \
} }, \
}, \
tsContext, \
tensorstore::OpenMode::create | tensorstore::OpenMode::delete_existing, \
tensorstore::ReadWriteMode::read_write); \
TS_EVAL_CHECK(openFuture); \
\
auto writeStore = openFuture.value(); \
auto * p = reinterpret_cast<typeName const *>(buffer); \
auto arr = tensorstore::Array(p, shape, tensorstore::c_order); \
auto writeFuture = tensorstore::Write(tensorstore::UnownedToShared(arr), writeStore); \
TS_EVAL_CHECK(writeFuture); \
}

void
Expand Down
Loading