Evaluate `tensorstoreToITKComponentType` at compile-time, using constexpr #68

N-Dekker · 2024-08-20T13:58:13Z

This pull request forces the compiler to evaluate the tensorstoreToITKComponentType calls at compile-time, both in OMEZarrNGFFImageIO::Read and OMEZarrNGFFImageIO::Write. It might yield a small performance improvement. Otherwise, it's at least a nice-to-have 😃

Might prevent compiler/code analysis warnings like "This function function-name could be marked constexpr if compile-time evaluation is desired (f.4)", https://learn.microsoft.com/en-us/cpp/code-quality/c26497?view=msvc-170

When reviewing the code changes, it may be helpful to skip whitespace changes: https://github.com/InsightSoftwareConsortium/ITKIOOMEZarrNGFF/pull/68/files?w=1

Following Scott Meyers, Effective Modern C++, 2014, "Use `constexpr` whenever possible".

Ensures that `tensorstoreToITKComponentType` is evaluated at compile-time, by introducing a constexpr variable template, `toITKComponentType<T>`.

N-Dekker · 2024-08-20T14:04:06Z

src/itkOMEZarrNGFFImageIO.cxx

@@ -602,11 +606,10 @@ OMEZarrNGFFImageIO::ReadImageInformation()
 }

 // We call tensorstoreToITKComponentType for each type.
-// Hopefully compiler will optimize it away via constant propagation and inlining.


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!

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

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?

tensorstoreToITKComponentType calls have many if-else branches.

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;

}

}

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)

The other PR that I submitted (Replace READ_ELEMENT_IF with variadic template, move functions into unnamed namespace #67) proposes to the sequence of READ_ELEMENT_IF calls. So then those if-else branches are also addressed 😃

dzenanz · 2024-08-20T14:15:08Z

src/itkOMEZarrNGFFImageIO.cxx

@@ -602,11 +606,10 @@ OMEZarrNGFFImageIO::ReadImageInformation()
 }

 // We call tensorstoreToITKComponentType for each type.
-// Hopefully compiler will optimize it away via constant propagation and inlining.


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

N-Dekker · 2024-08-20T15:08:00Z

src/itkOMEZarrNGFFImageIO.cxx

@@ -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())


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

Reported at Please allow compile-time evaluation of DataType::id() (constexpr), platform-independently google/tensorstore#189

Just fixed, by google/tensorstore@91ea2a2 😃

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?

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!

N-Dekker · 2024-08-20T16:19:20Z

src/itkOMEZarrNGFFImageIO.cxx

+template <typename T>
+static constexpr IOComponentEnum toITKComponentType = tensorstoreToITKComponentType(tensorstore::dtype_v<T>);


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? 🤔

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

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? 😸

N-Dekker added 2 commits August 20, 2024 15:23

STYLE: Declare component type conversion functions constexpr

63c4d91

Following Scott Meyers, Effective Modern C++, 2014, "Use `constexpr` whenever possible".

PERF: Call tensorstoreToITKComponentType at compile-time, by constexpr

6678cc4

Ensures that `tensorstoreToITKComponentType` is evaluated at compile-time, by introducing a constexpr variable template, `toITKComponentType<T>`.

N-Dekker force-pushed the constexpr-Type-conversion-functions branch from 5e723a5 to 6678cc4 Compare August 20, 2024 13:59

N-Dekker commented Aug 20, 2024

View reviewed changes

dzenanz approved these changes Aug 20, 2024

View reviewed changes

N-Dekker commented Aug 20, 2024

View reviewed changes

N-Dekker mentioned this pull request Aug 20, 2024

Please allow compile-time evaluation of DataType::id() (constexpr), platform-independently google/tensorstore#189

Closed

N-Dekker commented Aug 20, 2024

View reviewed changes

dzenanz mentioned this pull request Sep 12, 2024

ENH: Update the version of tensorstore we use #74

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Evaluate `tensorstoreToITKComponentType` at compile-time, using constexpr #68

Evaluate `tensorstoreToITKComponentType` at compile-time, using constexpr #68

N-Dekker commented Aug 20, 2024 •

edited

Loading

N-Dekker Aug 20, 2024

dzenanz Aug 20, 2024

N-Dekker Aug 20, 2024

dzenanz Aug 20, 2024

N-Dekker Aug 20, 2024

dzenanz Aug 20, 2024

N-Dekker Aug 20, 2024

dzenanz Aug 20, 2024

N-Dekker Aug 20, 2024

N-Dekker Aug 20, 2024

N-Dekker Sep 6, 2024

dzenanz Sep 6, 2024

N-Dekker Sep 6, 2024

N-Dekker Aug 20, 2024

dzenanz Aug 20, 2024

N-Dekker Aug 20, 2024

	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;
	}
	}

	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)

		template <typename T>
		static constexpr IOComponentEnum toITKComponentType = tensorstoreToITKComponentType(tensorstore::dtype_v<T>);

Evaluate tensorstoreToITKComponentType at compile-time, using constexpr #68

Are you sure you want to change the base?

Evaluate tensorstoreToITKComponentType at compile-time, using constexpr #68

Conversation

N-Dekker commented Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Evaluate `tensorstoreToITKComponentType` at compile-time, using constexpr #68

Evaluate `tensorstoreToITKComponentType` at compile-time, using constexpr #68

N-Dekker commented Aug 20, 2024 •

edited

Loading