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

[PowerPC] Forbid f128 SELECT_CC optimized into fsel #71497

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

ecnelises
Copy link
Member

fsel only supports f32 and f64.

@@ -8080,7 +8080,8 @@ SDValue PPCTargetLowering::LowerSELECT_CC(SDValue Op, SelectionDAG &DAG) const {
// For more information, see section F.3 of the 2.06 ISA specification.
// With ISA 3.0
if ((!DAG.getTarget().Options.NoInfsFPMath && !Flags.hasNoInfs()) ||
(!DAG.getTarget().Options.NoNaNsFPMath && !Flags.hasNoNaNs()))
(!DAG.getTarget().Options.NoNaNsFPMath && !Flags.hasNoNaNs()) ||
ResVT == MVT::f128)
Copy link
Contributor

Choose a reason for hiding this comment

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

does that mean fsel also does not support ppc_fp128?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we don't have instruction for ppc_fp128 fsel. The motivating case exists from 097a95f , which sets select_cc f128 as custom, while we don't have that lowering for ppc_fp128.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But 097a95f will not try to custom SELECT_CC to fsel for ppc_fp128, right? That commit is only for ieee long double.
Do we have other pattern that will generate fsel operating on ppc_fp128 type?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. PPCISD::FSEL is only generated in LowerSELECT_CC, while SELECT_CC ppc_fp128 is expand.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank-you!

Copy link
Collaborator

@chenzheng1030 chenzheng1030 left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me.
Please wait for one day in case @lei137 has other concerns. Thanks for fixing this.

@ecnelises ecnelises merged commit 426ad99 into llvm:main Nov 15, 2023
3 checks passed
@ecnelises ecnelises deleted the forbid_f128_fsel branch November 15, 2023 04:20
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
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.

None yet

3 participants