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

MacDue · 2025-02-20T09:22:29Z

Fixes ExpandFloatRes_FMODF when the DAG.getRoot() returns null
Adds missing SoftenFloatRes_FMODF legalization for MODF

With this DAGTypeLegalizer preserves root changes made during type legalization, which is needed to ensure the workaround for the expansion of nodes like FMODF and FFREXP is not undone:

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

Lines 2639 to 2646 in 41cece8

    
           // FIXME: Find a way to avoid updating the root. This is needed for x86, 
        
           // which uses a floating-point stack. If (for example) the node to be 
        
           // expanded has two results one floating-point which is returned by the 
        
           // call, and one integer result, returned via an output pointer. If only the 
        
           // integer result is used then the `CopyFromReg` for the FP result may be 
        
           // 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.

Apparently `DAG.getRoot()` can return null, so we need to check that case. Hopefully fixes: https://lab.llvm.org/buildbot/#/builders/72/builds/8406

llvmbot · 2025-02-20T09:23:03Z

@llvm/pr-subscribers-backend-arm
@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-backend-powerpc

Author: Benjamin Maxwell (MacDue)

Changes

Apparently DAG.getRoot() can return null, so we need to check that case. Hopefully fixes: https://lab.llvm.org/buildbot/#/builders/72/builds/8406

Full diff: https://github.com/llvm/llvm-project/pull/127976.diff

2 Files Affected:

(modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+3-1)
(modified) llvm/test/CodeGen/PowerPC/llvm.modf.ll (+22)

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 0a3210a10d394..8fd0de8d2fc44 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -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)
+                : CallChain;
     setRoot(NewRoot);
     // Ensure the new root is reachable from the results.
     Results[0] = getMergeValues({Results[0], NewRoot}, DL);
diff --git a/llvm/test/CodeGen/PowerPC/llvm.modf.ll b/llvm/test/CodeGen/PowerPC/llvm.modf.ll
index 69e3b22c7352c..a3f8a9907a46a 100644
--- a/llvm/test/CodeGen/PowerPC/llvm.modf.ll
+++ b/llvm/test/CodeGen/PowerPC/llvm.modf.ll
@@ -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
+}

sdesmalen-arm

The code change looks sensible to me, but it would be good if someone with more experience in PPC assembly can verify the CHECK lines.

arsenm · 2025-02-20T09:50:39Z

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

    SDValue NewRoot =
-        getNode(ISD::TokenFactor, DL, MVT::Other, getRoot(), CallChain);
+        OldRoot ? getNode(ISD::TokenFactor, DL, MVT::Other, OldRoot, CallChain)


Why is this null here?

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.

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

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.

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?

llvm/test/CodeGen/PowerPC/llvm.modf.ll

MacDue · 2025-02-20T12:24:07Z

llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp

+  // 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);


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.

MacDue · 2025-02-20T14:31:15Z

llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h

@@ -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);


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.

MacDue · 2025-02-20T19:41:17Z

I've created an alternate PR here: #128055, this version updates the expansion so it works without updating the root.

That PR also includes an explanation on why this unfortunate workaround is needed.

MacDue · 2025-02-21T11:27:42Z

Closing this in favor of #128055.

…128055) If a (two-result) node like `FMODF` or `FFREXP` is expanded to a library call, where said library has the function prototype like: `float(float, float*)` -- that is it returns a float from the call and via an output pointer. The first result of the node maps to the value returned by value and the second result maps to the value returned via the output pointer. If only the second result is used after the expansion, we hit an issue on x87 targets: ``` // Before expansion: t0, t1 = fmodf x return t1 // t0 is unused ``` Expanded result: ``` ptr = alloca ch0 = call modf ptr t0, ch1 = copy_from_reg, ch0 // t0 unused t1, ch2 = ldr ptr, ch1 return t1 ``` So far things are alright, but the DAGCombiner optimizes this to: ``` ptr = alloca ch0 = call modf ptr // copy_from_reg optimized out t1, ch1 = ldr ptr, ch0 return t1 ``` On most targets this is fine. The optimized out `copy_from_reg` is unused and is a NOP. However, x87 uses a floating-point stack, and if the `copy_from_reg` is optimized out it won't emit a pop needed to remove the unused result. The prior solution for this was to attach the chain from the `copy_from_reg` to the root, which did work, however, the root is not always available (it's set to null during legalize types). So the alternate solution in this patch is to replace the `copy_from_reg` with an `X86ISD::POP_FROM_X87_REG` within the X86 call lowering. This node is the same as `copy_from_reg` except this node makes it explicit that it may lower to an x87 FPU stack pop. Optimizations should be more cautious when handling this node than a normal CopyFromReg to avoid removing a required FPU stack pop. ``` ptr = alloca ch0 = call modf ptr t0, ch1 = pop_from_x87_reg, ch0 // t0 unused t1, ch2 = ldr ptr, ch1 return t1 ``` Using this node ensures a required x87 FPU pop is not removed due to the DAGCombiner. This is an alternate solution for #127976.

…lvm#128055) If a (two-result) node like `FMODF` or `FFREXP` is expanded to a library call, where said library has the function prototype like: `float(float, float*)` -- that is it returns a float from the call and via an output pointer. The first result of the node maps to the value returned by value and the second result maps to the value returned via the output pointer. If only the second result is used after the expansion, we hit an issue on x87 targets: ``` // Before expansion: t0, t1 = fmodf x return t1 // t0 is unused ``` Expanded result: ``` ptr = alloca ch0 = call modf ptr t0, ch1 = copy_from_reg, ch0 // t0 unused t1, ch2 = ldr ptr, ch1 return t1 ``` So far things are alright, but the DAGCombiner optimizes this to: ``` ptr = alloca ch0 = call modf ptr // copy_from_reg optimized out t1, ch1 = ldr ptr, ch0 return t1 ``` On most targets this is fine. The optimized out `copy_from_reg` is unused and is a NOP. However, x87 uses a floating-point stack, and if the `copy_from_reg` is optimized out it won't emit a pop needed to remove the unused result. The prior solution for this was to attach the chain from the `copy_from_reg` to the root, which did work, however, the root is not always available (it's set to null during legalize types). So the alternate solution in this patch is to replace the `copy_from_reg` with an `X86ISD::POP_FROM_X87_REG` within the X86 call lowering. This node is the same as `copy_from_reg` except this node makes it explicit that it may lower to an x87 FPU stack pop. Optimizations should be more cautious when handling this node than a normal CopyFromReg to avoid removing a required FPU stack pop. ``` ptr = alloca ch0 = call modf ptr t0, ch1 = pop_from_x87_reg, ch0 // t0 unused t1, ch2 = ldr ptr, ch1 return t1 ``` Using this node ensures a required x87 FPU pop is not removed due to the DAGCombiner. This is an alternate solution for llvm#127976.

[SDAG] Fix llvm.modf for ppc_fp128 (attempt two)

be65788

Apparently `DAG.getRoot()` can return null, so we need to check that case. Hopefully fixes: https://lab.llvm.org/buildbot/#/builders/72/builds/8406

MacDue requested a review from arsenm February 20, 2025 09:22

llvmbot added backend:PowerPC llvm:SelectionDAG SelectionDAGISel as well labels Feb 20, 2025

MacDue requested a review from sdesmalen-arm February 20, 2025 09:22

MacDue mentioned this pull request Feb 20, 2025

[SDAG] Add missing ppc_fp128 ExpandFloatRes legalization for modf #127895

Merged

sdesmalen-arm approved these changes Feb 20, 2025

View reviewed changes

arsenm reviewed Feb 20, 2025

View reviewed changes

Fixups

ae1d93f

MacDue commented Feb 20, 2025

View reviewed changes

Add missing soften for modf

b522657

llvmbot added the backend:ARM label Feb 20, 2025

MacDue changed the title ~~[SDAG] Fix llvm.modf for ppc_fp128 (attempt two)~~ [SDAG] Fix/add more legalization cases for FMODF Feb 20, 2025

MacDue commented Feb 20, 2025

View reviewed changes

MacDue requested review from arsenm, sdesmalen-arm and efriedma-quic February 20, 2025 14:31

MacDue mentioned this pull request Feb 20, 2025

[SDAG][X86] Remove hack needed to avoid missing x87 FPU stack pops #128055

Merged

MacDue closed this Feb 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

MacDue commented Feb 20, 2025 •

edited

Loading

Uh oh!

llvmbot commented Feb 20, 2025 •

edited

Loading

Uh oh!

sdesmalen-arm left a comment

Uh oh!

arsenm Feb 20, 2025

Uh oh!

MacDue Feb 20, 2025

Uh oh!

MacDue Feb 20, 2025

Uh oh!

MacDue Feb 20, 2025

Uh oh!

MacDue Feb 20, 2025 •

edited

Loading

Uh oh!

Uh oh!

MacDue Feb 20, 2025

Uh oh!

MacDue Feb 20, 2025

Uh oh!

MacDue commented Feb 20, 2025 •

edited

Loading

Uh oh!

MacDue commented Feb 21, 2025

Uh oh!

Uh oh!

	// FIXME: Find a way to avoid updating the root. This is needed for x86,
	// which uses a floating-point stack. If (for example) the node to be
	// expanded has two results one floating-point which is returned by the
	// call, and one integer result, returned via an output pointer. If only the
	// integer result is used then the `CopyFromReg` for the FP result may be
	// 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.

	// 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());

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

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

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

Uh oh!

Conversation

MacDue commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdesmalen-arm left a comment

Choose a reason for hiding this comment

Uh oh!

arsenm Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

MacDue Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

MacDue Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

MacDue Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

MacDue Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MacDue Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

MacDue Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

MacDue commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MacDue commented Feb 21, 2025

Uh oh!

Uh oh!

MacDue commented Feb 20, 2025 •

edited

Loading

llvmbot commented Feb 20, 2025 •

edited

Loading

MacDue Feb 20, 2025 •

edited

Loading

MacDue commented Feb 20, 2025 •

edited

Loading