Skip to content

[PowerPC][CodeGen] Expand ISD::AssertNoFPClass for ppc_fp128 #152357

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[PowerPC][CodeGen] Expand ISD::AssertNoFPClass for ppc_fp128 #152357

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
9 changes: 9 additions & 0 deletions llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1551,6 +1551,7 @@ void DAGTypeLegalizer::ExpandFloatResult(SDNode *N, unsigned ResNo) {
case ISD::VAARG: ExpandRes_VAARG(N, Lo, Hi); break;

case ISD::ConstantFP: ExpandFloatRes_ConstantFP(N, Lo, Hi); break;
case ISD::AssertNoFPClass: ExpandFloatRes_AssertNoFPClass(N, Lo, Hi); break;
case ISD::FABS: ExpandFloatRes_FABS(N, Lo, Hi); break;
case ISD::STRICT_FMINNUM:
case ISD::FMINNUM: ExpandFloatRes_FMINNUM(N, Lo, Hi); break;
Expand Down Expand Up @@ -1966,6 +1967,14 @@ void DAGTypeLegalizer::ExpandFloatRes_FNEG(SDNode *N, SDValue &Lo,
Hi = DAG.getNode(ISD::FNEG, dl, Hi.getValueType(), Hi);
}

void DAGTypeLegalizer::ExpandFloatRes_AssertNoFPClass(SDNode *N, SDValue &Lo,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possible that this mainly just applies to the ppcf128 type?

SDValue &Hi) {
SDLoc dl(N);
GetExpandedFloat(N->getOperand(0), Lo, Hi);
Lo = DAG.getNode(ISD::AssertNoFPClass, dl, Lo.getValueType(), Lo);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Lo = DAG.getNode(ISD::AssertNoFPClass, dl, Lo.getValueType(), Lo);

Hi = DAG.getNode(ISD::AssertNoFPClass, dl, Hi.getValueType(), Hi);
Comment on lines +1974 to +1975
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it's safe to directly take the value for both halves, and if it is it's only in double double cases like ppcf128. For ppcf128, it might not even apply to both halves. The safest option to get the compilation to stop failing is to drop the operation altogether and revisit the details of when it's preservable later

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at PPCTargetLowering::LowerIS_FPCLASS, it appears that for ppcf128 the class is determined by element 1. So I think propagating the info for element 1 is correct. I don't have any evidence about element 0, so I think it should not be propagated there. I am okay with not propagating the assert for both elements for now. I don't see any type tests or asserts so it's hard to be confident that only ppcf128 reaches here, so I think ppcf128 specific stuff like element 1 is class should be if protected and there be a general clear both path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah, I noticed I have a typo here.

As Roland mentioned, the class comes from the first element, which is what I meant:

SDValue PPCTargetLowering::LowerIS_FPCLASS(SDValue Op,
                                           SelectionDAG &DAG) const {
  assert(Subtarget.hasP9Vector() && "Test data class requires Power9");
  SDValue LHS = Op.getOperand(0);
  uint64_t RHSC = Op.getConstantOperandVal(1);
  SDLoc Dl(Op);
  FPClassTest Category = static_cast<FPClassTest>(RHSC);
  if (LHS.getValueType() == MVT::ppcf128) {
    // The higher part determines the value class.
    LHS = DAG.getNode(ISD::EXTRACT_ELEMENT, Dl, MVT::f64, LHS,
                      DAG.getConstant(1, Dl, MVT::i32));
  }

  return getDataClassTest(LHS, Category, Dl, DAG, Subtarget);
}

Unless I am misunderstanding, based on the two comments, propagating the information from element 1 is correct but it sounds like should just drop the operation for now.

@arsenm Dumb question, sorry - but what exactly does dropping the operation look like in this case? I assume the Hi and Lo would not be set then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Passing through the results of GetExpandedFloat directly to the final results, without constructing a new AssertNoFPClass

}

void DAGTypeLegalizer::ExpandFloatRes_FP_EXTEND(SDNode *N, SDValue &Lo,
SDValue &Hi) {
EVT NVT = TLI.getTypeToTransformTo(*DAG.getContext(), N->getValueType(0));
Expand Down
1 change: 1 addition & 0 deletions llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,6 +681,7 @@ class LLVM_LIBRARY_VISIBILITY DAGTypeLegalizer {
SDNode *N, RTLIB::Libcall LC, std::optional<unsigned> CallRetResNo = {});

// clang-format off
void ExpandFloatRes_AssertNoFPClass(SDNode *N, SDValue &Lo, SDValue &Hi);
void ExpandFloatRes_FABS (SDNode *N, SDValue &Lo, SDValue &Hi);
void ExpandFloatRes_FACOS (SDNode *N, SDValue &Lo, SDValue &Hi);
void ExpandFloatRes_FASIN (SDNode *N, SDValue &Lo, SDValue &Hi);
Expand Down
13 changes: 13 additions & 0 deletions llvm/test/CodeGen/PowerPC/nofpclass.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu < %s \
; RUN: | FileCheck %s
; RUN: llc -verify-machineinstrs -mtriple=powerpc64-ibm-aix-xcoff < %s \
; RUN: | FileCheck %s
Comment on lines +2 to +5
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu < %s \
; RUN: | FileCheck %s
; RUN: llc -verify-machineinstrs -mtriple=powerpc64-ibm-aix-xcoff < %s \
; RUN: | FileCheck %s
; RUN: llc -mtriple=powerpc64le-unknown-linux-gnu < %s | FileCheck %s
; RUN: llc -mtriple=powerpc64-ibm-aix-xcoff < %s | FileCheck %s


define ppc_fp128 @f(ppc_fp128 nofpclass(nan) %s) {
; CHECK-LABEL: f:
; CHECK: # %bb.0: # %entry
; CHECK-NEXT: blr
entry:
ret ppc_fp128 %s
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally would test with more masks, and some cases that demonstrate downstream effects of preserving it after legalization. That's best left to a follow up that tries to preserve it though