Skip to content

[SDAG] Fix/add more legalization cases for FMODF #127976

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 3 commits into from
Closed

[SDAG] Fix/add more legalization cases for FMODF #127976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be65788
[SDAG] Fix llvm.modf for ppc_fp128 (attempt two)
MacDue Feb 20, 2025
ae1d93f
Fixups
MacDue Feb 20, 2025
b522657
Add missing soften for modf
MacDue Feb 20, 2025
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
44 changes: 32 additions & 12 deletions llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ void DAGTypeLegalizer::SoftenFloatResult(SDNode *N, unsigned ResNo) {
case ISD::STRICT_FLDEXP: R = SoftenFloatRes_ExpOp(N); break;
case ISD::FFREXP: R = SoftenFloatRes_FFREXP(N); break;
case ISD::FSINCOS: R = SoftenFloatRes_FSINCOS(N); break;
case ISD::FMODF: R = SoftenFloatRes_FMODF(N); break;
case ISD::STRICT_FREM:
case ISD::FREM: R = SoftenFloatRes_FREM(N); break;
case ISD::STRICT_FRINT:
Expand Down Expand Up @@ -791,27 +792,35 @@ SDValue DAGTypeLegalizer::SoftenFloatRes_FFREXP(SDNode *N) {
return ReturnVal;
}

SDValue
DAGTypeLegalizer::SoftenFloatRes_UnaryWithTwoFPResults(SDNode *N,
RTLIB::Libcall LC) {
SDValue DAGTypeLegalizer::SoftenFloatRes_UnaryWithTwoFPResults(
SDNode *N, RTLIB::Libcall LC, std::optional<unsigned> CallRetResNo) {
assert(!N->isStrictFPOpcode() && "strictfp not implemented");
EVT VT = N->getValueType(0);

assert(VT == N->getValueType(1) &&
"expected both return values to have the same type");

if (!TLI.getLibcallName(LC))
return SDValue();

EVT NVT = TLI.getTypeToTransformTo(*DAG.getContext(), VT);
SDValue FirstResultSlot = DAG.CreateStackTemporary(NVT);
SDValue SecondResultSlot = DAG.CreateStackTemporary(NVT);

SDLoc DL(N);

TargetLowering::MakeLibCallOptions CallOptions;
std::array Ops{GetSoftenedFloat(N->getOperand(0)), FirstResultSlot,
SecondResultSlot};
std::array OpsVT{VT, FirstResultSlot.getValueType(),
SecondResultSlot.getValueType()};
SmallVector<SDValue, 3> Ops = {GetSoftenedFloat(N->getOperand(0))};
SmallVector<EVT, 3> OpsVT = {VT};

std::array<SDValue, 2> StackSlots;
for (auto [ResNum, _] : enumerate(N->values())) {
if (ResNum == CallRetResNo)
continue;
SDValue StackSlot = DAG.CreateStackTemporary(NVT);
Ops.push_back(StackSlot);
OpsVT.push_back(StackSlot.getValueType());
StackSlots[ResNum] = StackSlot;
}

TargetLowering::MakeLibCallOptions CallOptions;
// TODO: setTypeListBeforeSoften can't properly express multiple return types,
// but since both returns have the same type it should be okay.
CallOptions.setTypeListBeforeSoften({OpsVT}, VT, true);
Expand All @@ -825,8 +834,14 @@ DAGTypeLegalizer::SoftenFloatRes_UnaryWithTwoFPResults(SDNode *N,
MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), FrameIdx);
return DAG.getLoad(NVT, DL, Chain, StackSlot, PtrInfo);
};
SetSoftenedFloat(SDValue(N, 0), CreateStackLoad(FirstResultSlot));
SetSoftenedFloat(SDValue(N, 1), CreateStackLoad(SecondResultSlot));

for (auto [ResNum, SlackSlot] : enumerate(StackSlots)) {
if (CallRetResNo == ResNum) {
SetSoftenedFloat(SDValue(N, ResNum), ReturnVal);
continue;
}
SetSoftenedFloat(SDValue(N, ResNum), CreateStackLoad(SlackSlot));
}

return SDValue();
}
Expand All @@ -836,6 +851,11 @@ SDValue DAGTypeLegalizer::SoftenFloatRes_FSINCOS(SDNode *N) {
N, RTLIB::getSINCOS(N->getValueType(0)));
}

SDValue DAGTypeLegalizer::SoftenFloatRes_FMODF(SDNode *N) {
return SoftenFloatRes_UnaryWithTwoFPResults(
N, RTLIB::getMODF(N->getValueType(0)), /*CallRetResNo=*/0);
}

SDValue DAGTypeLegalizer::SoftenFloatRes_FREM(SDNode *N) {
return SoftenFloatRes_Binary(N, GetFPLibCall(N->getValueType(0),
RTLIB::REM_F32,
Expand Down
17 changes: 15 additions & 2 deletions llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,8 +435,21 @@ bool DAGTypeLegalizer::run() {
#endif
PerformExpensiveChecks();

// If the root changed (e.g. it was a dead load) update the root.
DAG.setRoot(Dummy.getValue());
// Get the value of the original root after type legalization.
SDValue Root = Dummy.getValue();

// Get the current root value, if it's not null combine it with the original
// root to prevent it being removed as a dead node.
if (SDValue LegalRoot = DAG.getRoot()) {
Root = DAG.getNode(ISD::TokenFactor, SDLoc(LegalRoot), MVT::Other, Root,
LegalRoot);
// The token_factor should not need any legalization (as both inputs have
// already been legalized).
Root->setNodeId(Processed);
}

// Restore the root.
DAG.setRoot(Root);
Comment on lines +438 to +452
Copy link
Member Author

Choose a reason for hiding this comment

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

Any thoughts? I can't think of an obvious way to avoid the need of the workaround for the expansion of modf and frexp, and I don't think it's safe to do if LegalizeTypes types just blindly ignores any changes to the root.


// Remove dead nodes. This is important to do for cleanliness but also before
// the checking loop below. Implicit folding by the DAG.getNode operators and
Expand Down
4 changes: 3 additions & 1 deletion llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,8 @@ class LLVM_LIBRARY_VISIBILITY DAGTypeLegalizer {
// Convert Float Results to Integer.
void SoftenFloatResult(SDNode *N, unsigned ResNo);
SDValue SoftenFloatRes_Unary(SDNode *N, RTLIB::Libcall LC);
SDValue SoftenFloatRes_UnaryWithTwoFPResults(SDNode *N, RTLIB::Libcall LC);
SDValue SoftenFloatRes_UnaryWithTwoFPResults(
SDNode *N, RTLIB::Libcall LC, std::optional<unsigned> CallRetResNo = {});
SDValue SoftenFloatRes_Binary(SDNode *N, RTLIB::Libcall LC);
SDValue SoftenFloatRes_MERGE_VALUES(SDNode *N, unsigned ResNo);
SDValue SoftenFloatRes_ARITH_FENCE(SDNode *N);
Expand Down Expand Up @@ -608,6 +609,7 @@ class LLVM_LIBRARY_VISIBILITY DAGTypeLegalizer {
SDValue SoftenFloatRes_ExpOp(SDNode *N);
SDValue SoftenFloatRes_FFREXP(SDNode *N);
SDValue SoftenFloatRes_FSINCOS(SDNode *N);
SDValue SoftenFloatRes_FMODF(SDNode *N);
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: SoftenFloatRes_FMODF and SoftenFloatRes_FFREXP are implicitly assuming the "missing pop" issue does not apply them (and do not attempt to implement the workaround). I think this is likely true, since these lower floats to integers, but I thought I'd point this out.

SDValue SoftenFloatRes_FREEZE(SDNode *N);
SDValue SoftenFloatRes_FREM(SDNode *N);
SDValue SoftenFloatRes_FRINT(SDNode *N);
Expand Down
4 changes: 3 additions & 1 deletion llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2644,8 +2644,10 @@ bool SelectionDAG::expandMultipleResultFPLibCall(
// optimized out. This prevents an FP stack pop from being emitted for it.
// Setting the root like this ensures there will be a use of the
// `CopyFromReg` chain, and ensures the FP pop will be emitted.
SDValue OldRoot = getRoot();
SDValue NewRoot =
getNode(ISD::TokenFactor, DL, MVT::Other, getRoot(), CallChain);
OldRoot ? getNode(ISD::TokenFactor, DL, MVT::Other, OldRoot, CallChain)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this null here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh, it's because of:

llvm-project/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp

Lines 209 to 211 in dc326d0

// The root of the dag may dangle to deleted nodes until the type legalizer is
// done. Set it to null to avoid confusion.
DAG.setRoot(SDValue());

Which then later ignores any changes to the root:

llvm-project/llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp

Lines 438 to 439 in dc326d0

// If the root changed (e.g. it was a dead load) update the root.
DAG.setRoot(Dummy.getValue());

Which I think would be incorrect for the workround the multiple-result expansions are doing.

Copy link
Member Author

Choose a reason for hiding this comment

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

(I've reverted the clang PR while I look into this further)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this will have to be updated to preserve the root created during legalize types, as otherwise the workaround needed for functions like modf and frexp when only the result from the output pointer is used won't work (in all cases), and I can't think of any obvious alternate solution here.

Copy link
Member Author

@MacDue MacDue Feb 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Well, whether or not it's okay to ignore the new root created by the expansion workround depends on if the ignored result is returned via some sort of stack, that needs to be popped regardless of if that value is used or not. That depends on the target, as far as I know it's only really x86 where that is an issue?

: CallChain;
setRoot(NewRoot);
// Ensure the new root is reachable from the results.
Results[0] = getMergeValues({Results[0], NewRoot}, DL);
Expand Down
Loading