Skip to content

[MIPS] match llvm.{min,max}num with {min,max}.fmt for R6 #89021

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

Merged
merged 1 commit into from
Apr 27, 2024

Conversation

Cyanoxygen
Copy link
Contributor

@Cyanoxygen Cyanoxygen commented Apr 17, 2024

  • The behavior is similar to UCOMISD on x86, which is also used to compare two fp values, specifically on handling of NaNs.
  • Update related tests regarding this change.
  • The further goal is to implement llvm.minimum and llvm.maximum intrinsics for MIPS R6 and Pre-R6.

Part of #64207

@wzssyqa
Copy link
Contributor

wzssyqa commented Apr 24, 2024

We need a real regression test for fmax/fmin. Please add a new one.
Maybe RISCV/float-maximum-minimum.ll is a good example.

@wzssyqa wzssyqa self-requested a review April 24, 2024 14:57
@Cyanoxygen Cyanoxygen force-pushed the mipsr6-match-fminmax branch from abe5343 to b50b5af Compare April 25, 2024 03:06
@Cyanoxygen
Copy link
Contributor Author

Maybe RISCV/float-maximum-minimum.ll is a good example.

That test's for llvm.fminimum and llvm.fmaximum, which is different than fminnum and fmaxnum. I can extend the test in #89907.

For the test in this PR, we definitely need a new one.

@Cyanoxygen Cyanoxygen force-pushed the mipsr6-match-fminmax branch from b50b5af to 5e31f72 Compare April 25, 2024 10:12
- The behavior is similar to UCOMISD on x86, which is also used to
  compare two fp values, specifically on handling of NaNs.
- Update related tests regarding this change.
- Add a new test. Note that this test do not cover pre-r6 targets since
  they are expanded into a libcall.
@Cyanoxygen Cyanoxygen force-pushed the mipsr6-match-fminmax branch from 5e31f72 to 189bed5 Compare April 25, 2024 15:16
@wzssyqa
Copy link
Contributor

wzssyqa commented Apr 27, 2024

@Cyanoxygen MSA has fmax.fmt/fmin.fmt, too.
Consider to support them?

@wzssyqa wzssyqa merged commit 7152194 into llvm:main Apr 27, 2024
3 of 4 checks passed
@Cyanoxygen
Copy link
Contributor Author

Cyanoxygen commented Apr 27, 2024

@Cyanoxygen MSA has fmax.fmt/fmin.fmt, too. Consider to support them?

Yes, I would like to try to look into that.

@wzssyqa
Copy link
Contributor

wzssyqa commented Apr 27, 2024

#90321 @Cyanoxygen

@wzssyqa
Copy link
Contributor

wzssyqa commented May 22, 2024

Ohh, this patch is not correct.

In https://llvm.org/docs/LangRef.html#llvm-minnum-intrinsic :

Unlike the IEEE-754 2008 behavior, this does not distinguish between signaling and quiet NaN inputs. If a target’s implementation follows the standard and returns a quiet NaN if either input is a signaling NaN, the intrinsic lowering is responsible for quieting the inputs to correctly return the non-NaN input (e.g. by using the equivalent of llvm.canonicalize).

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