Skip to content

Conversation

@farzonl
Copy link
Member

@farzonl farzonl commented Nov 21, 2024

  • For the HLSL intrinsic used const inline because that seems to be the pattern countbits
  • For the helper functions the pattern in hlsl_detail.h was to do constexpr like with bit_cast and enable_if_t So did the same here.
  • Distance in DXC is defined as Length(X-Y), So doing the same here.
  • Maybe this also means we need to move length into the header
  • This resolves all the DirectX specific parts of Implement the distance HLSL Function #99107
  • The codegen will be functionally correct for SPIRV, but will not emit the GLSL Distance opcode.
  • There are many potential solutions for that. Atm, we intend to address the GLSL specific ops with Inline SPIRV.

template <typename T>
constexpr __detail::enable_if_t<
__detail::is_same<float, T>::value || __detail::is_same<half, T>::value, T>
distance_impl(T X, T Y) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would we want to also put this in namespace __detail, or prefix the _impl versions with __ or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

moving implementations to a separate header makes sense. It would simplify our ordering problem and issues with type promotion that can pop up when using the builtins on scalars. There isn't much in the __detail namespace yet so I don't know what the rules are for putting things there.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't necessarily need to be moved to a different header to put it in a different namespace.

Wondering how far we can go with having lots of headers....would having one header per-builtin be reasonable? Would precompiled headers totally swallow any per-file overhead there?

@farzonl farzonl force-pushed the hlsl-distance branch 5 times, most recently from 717f5aa to c316336 Compare November 26, 2024 04:52
@farzonl farzonl force-pushed the hlsl-distance branch 5 times, most recently from 8950723 to 2711415 Compare January 7, 2025 01:57
Addressing RFC comments, replace LangBuiltin with TargetBuiltin
@farzonl farzonl closed this Jan 16, 2025
@farzonl farzonl deleted the hlsl-distance branch January 16, 2025 07:34
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.

2 participants