Skip to content

investigate how to use llvm.is.fpclass and replace int_dx_isinf with it #87777

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
Closed
farzonl opened this issue Apr 5, 2024 · 3 comments
Closed

investigate how to use llvm.is.fpclass and replace int_dx_isinf with it #87777

farzonl opened this issue Apr 5, 2024 · 3 comments
Assignees
@Icohedron
Labels
backend:DirectX question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead!

Comments

@farzonl
Copy link
Member

farzonl commented Apr 5, 2024

HLSL Intrinsic DXIL Op LLVM Intrinsic
[isinf] [IsInf][dx.IsInf] [llvm.is.fpclass]

Originally posted by @bogner in #87367 (comment)

Justin's lowering table shows we don't need our own intrinsic here
@damyanp damyanp moved this to Ready in HLSL Support Oct 9, 2024
@damyanp
Copy link
Contributor

damyanp commented Jan 23, 2025

@farzonl - is this still needed or are there other issues tracking this?

@farzonl farzonl mentioned this issue Apr 24, 2025
Open
@farzonl
Copy link
Member Author

farzonl commented Apr 24, 2025

@damyanp This is still need and will likely impact the design of #137209

@Icohedron Icohedron self-assigned this Apr 25, 2025
@Icohedron Icohedron moved this from Ready to Active in HLSL Support Apr 28, 2025
@Icohedron Icohedron mentioned this issue Apr 30, 2025
Merged
@Icohedron
Copy link
Contributor

Icohedron commented May 1, 2025 •
edited
Loading

@tex3d mentioned that the isinf and isnan ops are intended to not be removed when using fast math.
Therefore, we shouldn't use the builtin llvm.is.fpclass because it will be removed by fast math optimizations.
Having a dedicated intrinsic (such as int_dx_isinf) prevents fast math optimizations from removing it.

@Icohedron Icohedron closed this as not planned Won't fix, can't repro, duplicate, stale May 1, 2025
@github-project-automation github-project-automation bot moved this from Active to Closed in HLSL Support May 1, 2025
@Icohedron Icohedron mentioned this issue May 1, 2025
Closed
@EugeneZelenko EugeneZelenko added the question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead! label May 1, 2025
Icohedron added a commit that referenced this issue May 8, 2025
@Icohedron
[DirectX] Implement llvm.is.fpclass lowering for the fcNegZero FPCl…
7c366b0 
…assTest and the `IsNaN`, `IsInf`, `IsFinite`, `IsNormal` DXIL ops (#138048)

Fixes #137209

This PR:
- Adds a case to `expandIntrinsic()` in `DXILIntrinsicExpansion.cpp` to
expand the `Intrinsic::is_fpclass` in the case of
`FPClassTest::fcNegZero`
- Defines the `IsNaN`, `IsFinite`, `IsNormal` DXIL ops in `DXIL.td`
- Adds a case to `lowerIntrinsics()` in `DXILOpLowering.cpp` to handle
the lowering of `Intrinsic::is_fpclass` to the DXIL ops `IsNaN`,
`IsInf`, `IsFinite`, `IsNormal` when the FPClassTest is `fcNan`,
`fcInf`, `fcFinite`, and `fcNormal` respectively
- Creates a test `llvm/test/CodeGen/DirectX/is_fpclass.ll` to exercise
the intrinsic expansion and DXIL op lowering of `Intrinsic::is_fpclass`

~~A separate PR will be made to remove the now-redundant `dx_isinf`
intrinsic to address #87777.~~

A proper implementation for the lowering of the `llvm.is.fpclass`
intrinsic to handle all possible combinations of FPClassTest can be
implemented in a separate PR. This PR's implementation focuses primarily
on addressing the current use-cases for DirectML and HLSL intrinsics.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Icohedron Icohedron

Labels
backend:DirectX question A question, not bug report. Check out https://llvm.org/docs/GettingInvolved.html instead!
Projects
HLSL Support
Status: Closed
Milestone
No milestone
Development

No branches or pull requests
4 participants
@farzonl @Icohedron @damyanp @EugeneZelenko