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

map llvm.maxnum to NMax instead of FMax and llvm.minnum to NMin instead of FMin #87072

Closed
farzonl opened this issue Mar 29, 2024 · 0 comments · Fixed by #88009
Closed

map llvm.maxnum to NMax instead of FMax and llvm.minnum to NMin instead of FMin #87072

farzonl opened this issue Mar 29, 2024 · 0 comments · Fixed by #88009
Assignees
Labels
backend:SPIR-V HLSL HLSL Language Support

Comments

@farzonl
Copy link
Member

farzonl commented Mar 29, 2024

FMax (Floating-Point Maximum):

  • Result is y if x < y.
  • If both x and y are zeros, either x or y is the result.
  • If one operand is NaN (Not a Number), the result is the - other operand (the non-NaN operand).
  • If both operands are NaN, the result is undefined.

NMax (NaN-Aware Maximum):

  • Result is y if x < y.
  • If both x and y are zeros, either x or y is the result.
  • If one operand is NaN, the other operand is the result.
  • If both operands are NaN, the result is NaN.
FMax is undefined if one of the operands in NaN,
 so in DXC we use NMax to match HLSL max. 
It looks like that matches the LLVM instrinsic semantics, 
so I think this will be a straightforward SPIR-V backend change, 
but we do have to check that it doesn't break anything for the OpenCL side. 

Originally posted by @sudonatalie in #86844 (comment)

This issue is to track the work to SPIRVInstructionSelector.cpp and make sure changing the TargetOpcode to GL::FMin GL::FMax doesn't break OpenCL.
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp#L433C1-L438C67

I suspect we will be able to change

...
selectExtInst(ResVReg, ResType, I, CL::fmin, GL::FMin);
...
selectExtInst(ResVReg, ResType, I, CL::fmax, GL::FMax);

to

...
selectExtInst(ResVReg, ResType, I, CL::fmin, GL::NMin);
...
selectExtInst(ResVReg, ResType, I, CL::fmax, GL::NMax);

without effecting openCL.

@farzonl farzonl added backend:SPIR-V HLSL HLSL Language Support and removed new issue labels Mar 29, 2024
@sudonatalie sudonatalie self-assigned this Apr 2, 2024
sudonatalie added a commit that referenced this issue Apr 9, 2024
SPIR-V intsruction selection was mapping the LLVM float min/max
intrinsics to FMin and FMax respectively for GL/Vulkan environments,
which does not match the intrinsics' documented treatment of NaN
operands. This patch switches the mapping to the correctly matched NMin
and NMax operations.

Fixes #87072
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:SPIR-V HLSL HLSL Language Support
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants