Skip to content

SelectionDAG: Use qNaN constant if FCANONICALIZE not LegalOrCustom #104564

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

Closed
wants to merge 1 commit into from
Closed

SelectionDAG: Use qNaN constant if FCANONICALIZE not LegalOrCustom #104564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8622,8 +8622,16 @@ SDValue TargetLowering::expandFMINIMUMNUM_FMAXIMUMNUM(SDNode *Node,
// If MinMax is NaN, let's quiet it.
if (!Flags.hasNoNaNs() && !DAG.isKnownNeverNaN(LHS) &&
!DAG.isKnownNeverNaN(RHS)) {
SDValue MinMaxQuiet =
DAG.getNode(ISD::FCANONICALIZE, DL, VT, MinMax, Flags);
SDValue MinMaxQuiet;
if (isOperationLegalOrCustom(ISD::FCANONICALIZE, VT)) {
Copy link
Contributor

@arsenm arsenm Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is a different change from the default change and I also dont think should be done. The correct operation is a canonicalize. This just drops payload bits for no reason

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh this is just throwing away the value altogether. We don't know this is a nan

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only use it if MinMax UO SETUO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a different change from the default change and I also dont think should be done. The correct operation is a canonicalize. This just drops payload bits for no reason

This is only used if FCANONICALIZE is not available.

Copy link
Contributor

Choose a reason for hiding this comment

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

But a nan constant isn't a replacement for a canonicalize. You are breaking any non-nan inputs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But a nan constant isn't a replacement for a canonicalize. You are breaking any non-nan inputs

it won't break non-nan inputs, as we have

MinMax =
         DAG.getSelectCC(DL, MinMax, MinMax, MinMaxQuiet, MinMax, ISD::SETUO);

The MinMaxQuiet will be used only if MinMax UO MinMax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You also can't do a signaling nan check with fcmp in strictfp functions. This cannot raise an exception

Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You also can't do a signaling nan check with fcmp in strictfp functions. This cannot raise an exception

I don't think that these expandFXXXX can meet the requirements of strictfp.
I guess only the architectures which has the exactly same instructions can do so.
Or maybe we can only use the way of soft float.

Copy link
Contributor

Choose a reason for hiding this comment

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

But in the canonicalize case you can just use the canonicalize result, you don't need the additional compare and select

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. So let's close this PR.

MinMaxQuiet = DAG.getNode(ISD::FCANONICALIZE, DL, VT, MinMax, Flags);
} else {
// MIPS pre-R5 and HPPA use different encoding of qNaN and sNaN.
// ISD::FCANONICALIZE is supported by MIPS.
// HPPA is not supported by LLVM yet.
MinMaxQuiet =
Copy link
Contributor

Choose a reason for hiding this comment

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

A make_quiet would need to be is_signaling(x) ? qnan : x. Or preferably is_signaling(x) ? make_quiet(x) : x. But really should just use the canonicalize, and not workaround it not being implemented here

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 am not trying to use make_quiet here. As it is not easy to do is_signaling.
What I am trying to do is inNaN(x) ? qnan : x.

Copy link
Contributor

Choose a reason for hiding this comment

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

The lack of support for canonicalize on a target is no reason to have this code here. Targets need to implement operations. If you really want to hack around this, this needs to be in a dedicated make-quiet helper function not directly used in this one context

DAG.getConstantFP(APFloat::getQNaN(VT.getFltSemantics()), DL, VT);
}
MinMax =
DAG.getSelectCC(DL, MinMax, MinMax, MinMaxQuiet, MinMax, ISD::SETUO);
}
Expand Down
3 changes: 3 additions & 0 deletions llvm/lib/Target/X86/X86ISelLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,6 +608,7 @@ X86TargetLowering::X86TargetLowering(const X86TargetMachine &TM,
setOperationAction(ISD::FMAXNUM, VT, Action);
setOperationAction(ISD::FMINIMUM, VT, Action);
setOperationAction(ISD::FMAXIMUM, VT, Action);
setOperationAction(ISD::FCANONICALIZE, VT, Action);
setOperationAction(ISD::FSIN, VT, Action);
setOperationAction(ISD::FCOS, VT, Action);
setOperationAction(ISD::FSINCOS, VT, Action);
Expand Down Expand Up @@ -668,6 +669,8 @@ X86TargetLowering::X86TargetLowering(const X86TargetMachine &TM,
setOperationAction(ISD::FSIN , VT, Expand);
setOperationAction(ISD::FCOS , VT, Expand);
setOperationAction(ISD::FSINCOS, VT, Expand);

setOperationAction(ISD::FCANONICALIZE, VT, Expand);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a workaround with no actual expand implementation

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I figured it might be so, So I take it, I should continue to prepare my patch. I plan to implement similar to your AMD gpu implementation.

}

// Half type will be promoted by default.
Expand Down
Loading
Loading