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 1 commit
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
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
22 changes: 22 additions & 0 deletions llvm/test/CodeGen/PowerPC/llvm.modf.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,3 +328,25 @@ define { ppc_fp128, ppc_fp128 } @test_modf_ppcf128(ppc_fp128 %a) {
%result = call { ppc_fp128, ppc_fp128 } @llvm.modf.ppcf128(ppc_fp128 %a)
ret { ppc_fp128, ppc_fp128 } %result
}

define ppc_fp128 @test_modf_ppcf128_only_use_intergral(ppc_fp128 %a) {
; CHECK-LABEL: test_modf_ppcf128_only_use_intergral:
; CHECK: # %bb.0:
; CHECK-NEXT: mflr r0
; CHECK-NEXT: stdu r1, -48(r1)
; CHECK-NEXT: std r0, 64(r1)
; CHECK-NEXT: .cfi_def_cfa_offset 48
; CHECK-NEXT: .cfi_offset lr, 16
; CHECK-NEXT: addi r5, r1, 32
; CHECK-NEXT: bl modfl
; CHECK-NEXT: nop
; CHECK-NEXT: lfd f1, 32(r1)
; CHECK-NEXT: lfd f2, 40(r1)
; CHECK-NEXT: addi r1, r1, 48
; CHECK-NEXT: ld r0, 16(r1)
; CHECK-NEXT: mtlr r0
; CHECK-NEXT: blr
%result = call { ppc_fp128, ppc_fp128 } @llvm.modf.ppcf128(ppc_fp128 %a)
%result.1 = extractvalue { ppc_fp128, ppc_fp128 } %result, 1
ret ppc_fp128 %result.1
}