Skip to content

Conversation

@mikolaj-pirog
Copy link
Member

As in title. See here for more context: https://discourse.llvm.org/t/allowfpopfusion-vs-sdnodeflags-hasallowcontract/80909

Testing has been updated in previous PRs, so no testing fail should be seen

@llvmbot
Copy link
Member

llvmbot commented Nov 9, 2025

@llvm/pr-subscribers-backend-x86

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

Changes

As in title. See here for more context: https://discourse.llvm.org/t/allowfpopfusion-vs-sdnodeflags-hasallowcontract/80909

Testing has been updated in previous PRs, so no testing fail should be seen


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

1 Files Affected:

  • (modified) llvm/lib/Target/X86/X86ISelLowering.cpp (+4-9)
diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp
index 05a854a0bf3fa..68dca58287c59 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -8434,7 +8434,6 @@ static bool isFMAddSubOrFMSubAdd(const X86Subtarget &Subtarget,
   // or MUL + ADDSUB to FMADDSUB.
   const TargetOptions &Options = DAG.getTarget().Options;
   bool AllowFusion =
-      Options.AllowFPOpFusion == FPOpFusion::Fast ||
       (AllowSubAddOrAddSubContract && Opnd0->getFlags().hasAllowContract());
   if (!AllowFusion)
     return false;
@@ -54160,11 +54159,7 @@ static SDValue combineFMulcFCMulc(SDNode *N, SelectionDAG &DAG,
 //  FADD(A, FMA(B, C, 0)) and FADD(A, FMUL(B, C)) to FMA(B, C, A)
 static SDValue combineFaddCFmul(SDNode *N, SelectionDAG &DAG,
                                 const X86Subtarget &Subtarget) {
-  auto AllowContract = [&DAG](const SDNodeFlags &Flags) {
-    return DAG.getTarget().Options.AllowFPOpFusion == FPOpFusion::Fast ||
-           Flags.hasAllowContract();
-  };
-
+  bool AllowContract = N->getFlags().hasAllowContract();
   auto HasNoSignedZero = [&DAG](const SDNodeFlags &Flags) {
     return DAG.getTarget().Options.NoSignedZerosFPMath ||
            Flags.hasNoSignedZeros();
@@ -54177,7 +54172,7 @@ static SDValue combineFaddCFmul(SDNode *N, SelectionDAG &DAG,
   };
 
   if (N->getOpcode() != ISD::FADD || !Subtarget.hasFP16() ||
-      !AllowContract(N->getFlags()))
+      !AllowContract)
     return SDValue();
 
   EVT VT = N->getValueType(0);
@@ -54188,14 +54183,14 @@ static SDValue combineFaddCFmul(SDNode *N, SelectionDAG &DAG,
   SDValue RHS = N->getOperand(1);
   bool IsConj;
   SDValue FAddOp1, MulOp0, MulOp1;
-  auto GetCFmulFrom = [&MulOp0, &MulOp1, &IsConj, &AllowContract,
+  auto GetCFmulFrom = [&MulOp0, &MulOp1, &IsConj, AllowContract,
                        &IsVectorAllNegativeZero,
                        &HasNoSignedZero](SDValue N) -> bool {
     if (!N.hasOneUse() || N.getOpcode() != ISD::BITCAST)
       return false;
     SDValue Op0 = N.getOperand(0);
     unsigned Opcode = Op0.getOpcode();
-    if (Op0.hasOneUse() && AllowContract(Op0->getFlags())) {
+    if (Op0.hasOneUse() && AllowContract) {
       if ((Opcode == X86ISD::VFMULC || Opcode == X86ISD::VFCMULC)) {
         MulOp0 = Op0.getOperand(0);
         MulOp1 = Op0.getOperand(1);

@github-actions
Copy link

github-actions bot commented Nov 9, 2025

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

@phoebewang
Copy link
Contributor

Should we warn fp-contract is not used on X86?

@mikolaj-pirog
Copy link
Member Author

Should we warn fp-contract is not used on X86?

It's not used by the X86 backend, but target independent code still uses it, so IR without contract on IR but with --fp-contract=fast on CLI may still emit FMA in the end

@phoebewang
Copy link
Contributor

Should we warn fp-contract is not used on X86?

It's not used by the X86 backend, but target independent code still uses it, so IR without contract on IR but with --fp-contract=fast on CLI may still emit FMA in the end

I think that would be a behavioral change for those who used --fp-contract=fast before. So ideal, they should be reminded not to use it any more.

Copy link
Contributor

@phoebewang phoebewang left a comment

Choose a reason for hiding this comment

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

LGTM.

@mikolaj-pirog mikolaj-pirog merged commit 20034ba into llvm:main Nov 13, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants