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.

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

[clang][docs] Revise documentation for __builtin_reduce_(max|min). #114637

Closed
wants to merge 3 commits into from

Conversation

c8ef
Copy link
Contributor

@c8ef c8ef commented Nov 2, 2024

The function operation described in the document did not match its actual semantic meaning, this patch resolved the problem.

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Nov 2, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 2, 2024

@llvm/pr-subscribers-clang

Author: None (c8ef)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/114637.diff

1 Files Affected:

  • (modified) clang/docs/LanguageExtensions.rst (+2-6)
diff --git a/clang/docs/LanguageExtensions.rst b/clang/docs/LanguageExtensions.rst
index 10232ff41da15a..b7676bab623ef8 100644
--- a/clang/docs/LanguageExtensions.rst
+++ b/clang/docs/LanguageExtensions.rst
@@ -745,12 +745,8 @@ Let ``VT`` be a vector type and ``ET`` the element type of ``VT``.
 ======================================= ====================================================================== ==================================
          Name                            Operation                                                              Supported element types
 ======================================= ====================================================================== ==================================
- ET __builtin_reduce_max(VT a)           return x or y, whichever is larger; If exactly one argument is         integer and floating point types
-                                         a NaN, return the other argument. If both arguments are NaNs,
-                                         fmax() return a NaN.
- ET __builtin_reduce_min(VT a)           return x or y, whichever is smaller; If exactly one argument           integer and floating point types
-                                         is a NaN, return the other argument. If both arguments are
-                                         NaNs, fmax() return a NaN.
+ ET __builtin_reduce_max(VT a)           return the largest element of the vector.                              integer and floating point types
+ ET __builtin_reduce_min(VT a)           return the smallest element of the vector.                             integer and floating point types
  ET __builtin_reduce_add(VT a)           \+                                                                     integer types
  ET __builtin_reduce_mul(VT a)           \*                                                                     integer types
  ET __builtin_reduce_and(VT a)           &                                                                      integer types

@arsenm arsenm added the floating-point Floating-point math label Nov 2, 2024
@c8ef c8ef requested review from fhahn and RKSimon November 3, 2024 01:29
is a NaN, return the other argument. If both arguments are
NaNs, fmax() return a NaN.
ET __builtin_reduce_max(VT a) return the largest element of the vector. integer and floating point types
ET __builtin_reduce_min(VT a) return the smallest element of the vector. integer and floating point types
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please can you add back the NaN semantics - AFAICT they should always return the largest/smallest non-NAN value if any vector element is non-NAN, otherwise it could return any of the NAN values in the (all NAN) vector. There might be a better way of phrasing that......

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LLVM LangRef states: "This instruction has the same comparison semantics as the ‘llvm.maxnum.*’ intrinsic." Can I add a link to https://llvm.org/docs/LangRef.html#vector-reduction-intrinsics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add pointers to the floating point min/max function.

Copy link
Contributor

@arsenm arsenm Nov 5, 2024

Choose a reason for hiding this comment

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

Do we have any kind of conformance testing for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if we had any conformance testing before, but since this is just a document change and the patch is keeping the clang builtin document aligned with the llvm intrinsic document, maybe there's no need to add extra tests here?

@c8ef c8ef requested a review from RKSimon November 3, 2024 15:29
floating point, this function has the same comparison semantics as
``__builtin_reduce_maximum``.
ET __builtin_reduce_min(VT a) return the smallest element of the vector. If the element type is integer and floating point types
floating point, this function has the same comparison semantics as
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the implementation, this is going to use reduce_fmax intrinsic, which is not the same as reduce_maximum

I think this builtin trying to support floating point is an anti-feature, and if it has to, it should have matched minimum/maximum. But that's not what the implementation is doing

This comment was marked as outdated.

This comment was marked as outdated.

@c8ef c8ef requested a review from arsenm November 6, 2024 00:53
@c8ef
Copy link
Contributor Author

c8ef commented Nov 7, 2024

I apologize for my earlier mistake. The function __builtin_reduce_max behaves differently from __builtin_reduce_maximum when working with floating point numbers. The former will only return nan if all elements are nan, while the latter will return nan if any element in the vector is nan. The most recent patch corrects the wording error and restores the nan semantics. Please take another look.
@arsenm @RKSimon

@c8ef c8ef closed this Nov 9, 2024
@c8ef c8ef deleted the reduce branch November 9, 2024 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category floating-point Floating-point math
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants