[SelectionDAG] Copy FP flags when visiting a binary instruction.

[vchuravy]

Summary

We noticed in Julia that the sequence below no longer turned into
a sequence of FMA instructions in LLVM 7+, but it did in LLVM 6.

%29 = fmul contract <4 x double> %wide.load, %wide.load16
%30 = fmul contract <4 x double> %wide.load13, %wide.load17
%31 = fmul contract <4 x double> %wide.load14, %wide.load18
%32 = fmul contract <4 x double> %wide.load15, %wide.load19
%33 = fadd fast <4 x double> %vec.phi, %29
%34 = fadd fast <4 x double> %vec.phi10, %30
%35 = fadd fast <4 x double> %vec.phi11, %31
%36 = fadd fast <4 x double> %vec.phi12, %32

Unlike Clang, Julia doesn't set the unsafe-fp-math=true function
attribute, but rather emits more local instruction flags.

This partially undoes https://reviews.llvm.org/D46854 and if required I can try to minimize the test further.

[merge_guards_bot]

Unit tests: pass. 60868 tests passed, 0 failed and 726 were skipped.

clang-format: pass.

Build artifacts: console-log.txt, CMakeCache.txt, test-results.xml, diff.json

[spatel]

It's not clear to me where the bug is. I see this debug output:

Detected a reduction operation:  %34 = fadd fast <4 x double> %vec.phi, %30
Creating new node: t36: v4f64 = fadd vector-reduction t35, t30 <-- dropped all other FMF

So that seems like we just accidentally cleared FMF by setting vector-reduction. If so, then a better fix would be after line 3127?

if (isVectorReductionOp(&I)) {
  Flags.setVectorReduction(true);
  LLVM_DEBUG(dbgs() << "Detected a reduction operation:" << I << "\n");
}

Reducing the test case would be good - no need to unroll 4x vector.body?

[vchuravy]

• reduce test
• update test

[vchuravy]

In D71495#1788333, @spatel wrote:

It's not clear to me where the bug is. I see this debug output:

Detected a reduction operation:  %34 = fadd fast <4 x double> %vec.phi, %30
Creating new node: t36: v4f64 = fadd vector-reduction t35, t30 <-- dropped all other FMF

So that seems like we just accidentally cleared FMF by setting vector-reduction. If so, then a better fix would be after line 3127?

We dropped all FMF because we created a new SDNodeFlags Flags and then create a new node with those flags and replace the input node with the new one.
Setting the vector reduction doesn't clear the FMF, so where we copy them doesn't matter much. I can of course move the copying of the FMF after setting the vector reduction flag.

I reduced the test as much as possible using llvm-reduce, I had to check whether a patched binary produced and fma and a unpatched binary not yet produced a fma.
If the test is missing the middle block the fma is generated in either case.

[merge_guards_bot]

Unit tests: pass. 61005 tests passed, 0 failed and 728 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[spatel]

In D71495#1790020, @vchuravy wrote:

In D71495#1788333, @spatel wrote:

It's not clear to me where the bug is. I see this debug output:

Detected a reduction operation:  %34 = fadd fast <4 x double> %vec.phi, %30
Creating new node: t36: v4f64 = fadd vector-reduction t35, t30 <-- dropped all other FMF

So that seems like we just accidentally cleared FMF by setting vector-reduction. If so, then a better fix would be after line 3127?

We dropped all FMF because we created a new SDNodeFlags Flags and then create a new node with those flags and replace the input node with the new one.
Setting the vector reduction doesn't clear the FMF, so where we copy them doesn't matter much. I can of course move the copying of the FMF after setting the vector reduction flag.

Ok, I see how this fails now - back in the parent SelectionDAGBuilder::visit() function, we intersect flags for nodes that already have flags, so we are intersecting 'fast' with 'vector-reduction' which is nothing.

I reduced the test as much as possible using llvm-reduce, I had to check whether a patched binary produced and fma and a unpatched binary not yet produced a fma.
If the test is missing the middle block the fma is generated in either case.

We have to trigger isVectorReductionOp() to be true, but that doesn't require separate basic blocks. I added a further reduced test here:
rG13d30bd54b8b

That covers the only code path for this bug, so you should be able to rebase and update that test file by running utils/update_llc_test_checks.py.

[mcberg2017]

When I looked at this recently as well, it looked like intersect was were we lost our flags.

[spatel]

In D71495#1791522, @mcberg2017 wrote:

When I looked at this recently as well, it looked like intersect was were we lost our flags.

Thanks. I don't see much downside to the fix as proposed currently, but let me know if you agree. If we take the flags from the IR instruction here, it will probably be redundant with the propagation in visit(). Or we can pull that flags copying to line 3128 and avoid intersecting flags so much. I doubt this makes any difference to overall codegen time.

[mcberg2017]

I like the notion of avoiding/reducing intersect context hits by placing it in the vector reduction scope (line 3128).

[vchuravy]

update PR

[merge_guards_bot]

Unit tests: pass. 61083 tests passed, 0 failed and 728 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[vchuravy]

clang-format

[merge_guards_bot]

Unit tests: pass. 61083 tests passed, 0 failed and 728 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[spatel]

LGTM - thanks!

[spatel]

llvm/test/CodeGen/X86/fmf-reduction.ll

4

remove 'FIXME'

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

if (auto *OFBinOp = dyn_cast<OverflowingBinaryOperator>(&I)) {
     Flags.setNoUnsignedWrap(OFBinOp->hasNoUnsignedWrap());
   }
   if (auto *ExactOp = dyn_cast<PossiblyExactOperator>(&I)) {
     Flags.setExact(ExactOp->isExact());
   }
   if (isVectorReductionOp(&I)) {
     Flags.setVectorReduction(true);
     LLVM_DEBUG(dbgs() << "Detected a reduction operation:" << I << "\n");
+
+    // If no flags are set we will propagate the incoming flags, if any flags
+    // are set, we will intersect them with the incoming flag and so we need to
+    // copy the FMF flags here.
+    if (auto *FPOp = dyn_cast<FPMathOperator>(&I)) {
+      Flags.copyFMF(*FPOp);
+    }
   }
 
   SDValue Op1 = getValue(I.getOperand(0));
   SDValue Op2 = getValue(I.getOperand(1));
   SDValue BinNodeValue = DAG.getNode(Opcode, getCurSDLoc(), Op1.getValueType(),
                                      Op1, Op2, Flags);
   setValue(&I, BinNodeValue);
 }

llvm/test/CodeGen/X86/fmf-reduction.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=x86_64-- -mattr=fma | FileCheck %s
 
-; FIXME: Propagation of IR FMF should not drop flags when adding the DAG reduction flag.
+; Propagation of IR FMF should not drop flags when adding the DAG reduction flag.
 ; This should include an FMA instruction, not separate FMUL/FADD.
 
 define double @julia_dotf(<4 x double> %x, <4 x double> %y, <4 x double> %z, i1 %t3) {
 ; CHECK-LABEL: julia_dotf:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    vmulpd %ymm1, %ymm0, %ymm0
-; CHECK-NEXT:    vaddpd %ymm0, %ymm2, %ymm0
+; CHECK-NEXT:    vfmadd213pd {{.*#+}} ymm0 = (ymm1 * ymm0) + ymm2
 ; CHECK-NEXT:    vextractf128 $1, %ymm0, %xmm1
 ; CHECK-NEXT:    vaddpd %xmm1, %xmm0, %xmm0
 ; CHECK-NEXT:    vpermilpd {{.*#+}} xmm1 = xmm0[1,0]
 ; CHECK-NEXT:    vaddsd %xmm1, %xmm0, %xmm0
 ; CHECK-NEXT:    vzeroupper
 ; CHECK-NEXT:    retq
   %t1 = fmul contract <4 x double> %x, %y
   %t2 = fadd fast <4 x double> %z, %t1
   %rdx.shuf = shufflevector <4 x double> %t2, <4 x double> undef, <4 x i32> <i32 2, i32 3, i32 undef, i32 undef>
   %bin.rdx22 = fadd fast <4 x double> %t2, %rdx.shuf
   %rdx.shuf23 = shufflevector <4 x double> %bin.rdx22, <4 x double> undef, <4 x i32> <i32 1, i32 undef, i32 undef, i32 undef>
   %bin.rdx24 = fadd fast <4 x double> %bin.rdx22, %rdx.shuf23
   %t4 = extractelement <4 x double> %bin.rdx24, i32 0
   ret double %t4
 }