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 2 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
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/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
105 changes: 105 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,108 @@ 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
}

define ppc_fp128 @test_modf_ppcf128_only_use_fractional(ppc_fp128 %a) {
; CHECK-LABEL: test_modf_ppcf128_only_use_fractional:
; 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: 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, 0
ret ppc_fp128 %result.1
}

define { ppc_fp128, ppc_fp128 } @test_modf_ppcf128_tail_call(ppc_fp128 %a) {
; CHECK-LABEL: test_modf_ppcf128_tail_call:
; 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 f3, 32(r1)
; CHECK-NEXT: lfd f4, 40(r1)
; CHECK-NEXT: addi r1, r1, 48
; CHECK-NEXT: ld r0, 16(r1)
; CHECK-NEXT: mtlr r0
; CHECK-NEXT: blr
%result = tail 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_tail_call(ppc_fp128 %a) {
; CHECK-LABEL: test_modf_ppcf128_only_use_intergral_tail_call:
; 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 = tail 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
}

define ppc_fp128 @test_modf_ppcf128_only_use_fractional_tail_call(ppc_fp128 %a) {
; CHECK-LABEL: test_modf_ppcf128_only_use_fractional_tail_call:
; 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: addi r1, r1, 48
; CHECK-NEXT: ld r0, 16(r1)
; CHECK-NEXT: mtlr r0
; CHECK-NEXT: blr
%result = tail call { ppc_fp128, ppc_fp128 } @llvm.modf.ppcf128(ppc_fp128 %a)
%result.1 = extractvalue { ppc_fp128, ppc_fp128 } %result, 0
ret ppc_fp128 %result.1
}