[SelectionDAGBuilder] Propagate fast-math flags to fpext

[mikolaj-pirog]

As in title. Without this, fpext behaves in selectionDAG as always having no fast-math flags.

[llvmbot]

@llvm/pr-subscribers-backend-powerpc

@llvm/pr-subscribers-llvm-selectiondag

Author: Mikołaj Piróg (mikolaj-pirog)

Changes

As in title. Without this, fpext behaves in selectionDAG as always having no fmf flags.

---

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

1 Files Affected:

• (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (+4-1)

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 9baf72b266aa7..16f555b16a621 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -3976,7 +3976,10 @@ void SelectionDAGBuilder::visitFPExt(const User &I) {
   SDValue N = getValue(I.getOperand(0));
   EVT DestVT = DAG.getTargetLoweringInfo().getValueType(DAG.getDataLayout(),
                                                         I.getType());
-  setValue(&I, DAG.getNode(ISD::FP_EXTEND, getCurSDLoc(), DestVT, N));
+  SDNodeFlags Flags;
+  if (auto *TruncInst = dyn_cast<FPMathOperator>(&I))
+    Flags.copyFMF(*TruncInst);
+  setValue(&I, DAG.getNode(ISD::FP_EXTEND, getCurSDLoc(), DestVT, N, Flags));
 }
 
 void SelectionDAGBuilder::visitFPToUI(const User &I) {

[mikolaj-pirog]

I'm seeing no testing failures on this one, meaning no code path checks in meaningful way fmf on fpext nodes. I run into this issue while working on strengthening fmf semantics

[arsenm]

Should be testable? fpext flags are new, I'd assume that would have covered this?

[mikolaj-pirog]

Should be testable? fpext flags are new, I'd assume that would have covered this?

I don't think we have explicit testing for this stuff (i.e. if flags propagate correctly on DAG construction), I couldn't find an easy to test it either. I believe other flags get naturally tested if they work with tests for respective transformations. Would writing a unittest for it be an overkill?

I've filled a PR that requires this fixes, so that could be repurposed as a test for it

[arsenm]

Should be testable? fpext flags are new, I'd assume that would have covered this?

I don't think we have explicit testing for this stuff (i.e. if flags propagate correctly on DAG construction), I couldn't find an easy to test it either. I believe other flags get naturally tested if they work with tests for respective transformations. Would writing a unittest for it be an overkill?

The brute force way would be use -stop-after=finalize-isel and check the MIR, but it would be better to have a transform dependent on the flags

I've filled a PR that requires this fixes, so that could be repurposed as a test for it

That one's big and not obviously related to fpext, I'd rather test it here. Is there some other context the flag will do something?

[mikolaj-pirog]

Should be testable? fpext flags are new, I'd assume that would have covered this?

I don't think we have explicit testing for this stuff (i.e. if flags propagate correctly on DAG construction), I couldn't find an easy to test it either. I believe other flags get naturally tested if they work with tests for respective transformations. Would writing a unittest for it be an overkill?

The brute force way would be use -stop-after=finalize-isel and check the MIR, but it would be better to have a transform dependent on the flags

I've filled a PR that requires this fixes, so that could be repurposed as a test for it

That one's big and not obviously related to fpext, I'd rather test it here. Is there some other context the flag will do something?

Some combines peek through FP_EXTEND to perform fmf rewrite, but none checks if FP_EXTEND has any flags. I could add this check to one of the combines and update the tests. DAGCombiner::isContractableFMUL is one of these combines.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[mikolaj-pirog]

Should be testable? fpext flags are new, I'd assume that would have covered this?

I don't think we have explicit testing for this stuff (i.e. if flags propagate correctly on DAG construction), I couldn't find an easy to test it either. I believe other flags get naturally tested if they work with tests for respective transformations. Would writing a unittest for it be an overkill?

The brute force way would be use -stop-after=finalize-isel and check the MIR, but it would be better to have a transform dependent on the flags

I've filled a PR that requires this fixes, so that could be repurposed as a test for it

That one's big and not obviously related to fpext, I'd rather test it here. Is there some other context the flag will do something?

The FPEXT in fdiv from other PR is a good candidate for testing this -- I've included checking arcp for FPEXT node here. One test caught the difference -- would that suffice for testing?

[bjope]

TruncInst is confusing here (I guess it has been copied from somewhere). You could perhaps all it Operator or FPExtInst or something, but it seems like FPOp is a common variable name when dyn_cast-ing to a FPMathOperator. So I would suggest FPOp.