This is an archive of the discontinued LLVM Phabricator instance.

[VP] fm flag transfer to SDNodes [VE] VVP_FMA fusion
Needs ReviewPublic

Authored by simoll on Mar 15 2022, 9:33 AM.

Details

Summary

On the VP side: Fast-math flag transfer from VP intrinsics to VP SDNodes. Tested through the VE parts of this patch.
For VE: Custom fusion code for VVP_FMA. Yes, there could/should be a generic DAGCombiner path for this, however, this is just the first in a series of VE-specific combiner patterns.

Diff Detail

Event Timeline

simoll created this revision.Mar 15 2022, 9:33 AM
simoll requested review of this revision.Mar 15 2022, 9:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2022, 9:33 AM
ABataev added inline comments.
llvm/include/llvm/IR/Operator.h
306

Better to expand auto here to the actual type.

simoll updated this revision to Diff 415511.Mar 15 2022, 11:12 AM
simoll marked an inline comment as done.

explicit Optional<unsigned>

craig.topper added inline comments.Mar 15 2022, 6:33 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7530

Is this correct if the getNode call CSEs with an existing node? Should probably pass the flags to getNode.

simoll updated this revision to Diff 415770.Mar 16 2022, 3:53 AM
simoll marked an inline comment as done.

Pass NodeFlags in getNode.
Rebased.

simoll updated this revision to Diff 416089.Mar 17 2022, 1:32 AM

Fixing an SelectionDAG bug where Flags wouldn't be set in some cases of SelectionDAG::getNode. This bug fix may have (positive) trickle down effects on other targets.

Fixing an SelectionDAG bug where Flags wouldn't be set in some cases of SelectionDAG::getNode. This bug fix may have (positive) trickle down effects on other targets.

Thanks!

The target independent parts look good to me. I didn't look too closely at the VE changes.

craig.topper added inline comments.Mar 17 2022, 10:26 AM
llvm/lib/Target/VE/CMakeLists.txt
30

Should this be alphabetized before VVPISelLowering?

llvm/lib/Target/VE/VVPCombine.cpp
30

Do you need to check that the FADD is contractable too? I think that's what DAGCombiner.cpp does but the different ways of specifiying fast math make it hard to be sure.

simoll updated this revision to Diff 416441.Mar 18 2022, 3:22 AM
simoll marked 2 inline comments as done.
  • alphabetic file order in the CMakeLists.txt
  • unchanged, just to reiterate: contract only checked on the fmul, not on the fadd.
  • formatting
simoll added inline comments.Mar 18 2022, 5:28 AM
llvm/lib/Target/VE/VVPCombine.cpp
30

The specification isn't clear on this; from the LangRef:

contract

    Allow floating-point contraction (e.g. fusing a multiply followed by an addition into a fused multiply-and-add). This does not enable reassociating to form arbitrary contractions. For example, (a*b) + (c*d) + e can not be transformed into (a*b) + ((c*d) + e) to create two fma operations.

I agree with you. Intuitively, the contract only needs to be on the node that is contracted into another node. Since contraction (in the sense of FMA) means removing the implicit rounding step after the fmul. The rounding after the fadd is unaffacted by this.