Page MenuHomePhabricator

[SelectionDAGBuilder] Pass fast math flags to most of VP SDNodes.
ClosedPublic

Authored by fakepaper56 on May 14 2022, 12:59 AM.

Details

Summary

The patch does not pass math flags to float VPCmpIntrinsics because LLParser
could not identify float VPCmpIntrinsics as FPMathOperators.

Diff Detail

Event Timeline

fakepaper56 created this revision.May 14 2022, 12:59 AM
fakepaper56 requested review of this revision.May 14 2022, 12:59 AM

Rename class form [SelectionDAGBuilder] to [VP].

fakepaper56 retitled this revision from [SelectionDAGBuilder] Pass fast math flags to VP SDNodes. to [VP] Pass fast math flags to VP SDNodes..May 14 2022, 1:03 AM

I would split this patch. One for vp.fcmp and one for the other operations. The LLParser issue is concerning and may delay the vp.fcmp support. That shouldn't block the other nodes.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7603

If we hit this line on an FP operation with fast math flags, would we add fast math flags to an ISD::ZERO_EXTEND?

Might be safer to pass the flags to the DAG.getNode call in the default case.

llvm/test/CodeGen/RISCV/rvv/fast-math-flags-vp.ll
3 ↗(On Diff #429415)

declartion -> declaration

The update removes the part of vp.fcmp support and using argument of DAG.getNode to pass flags.

fakepaper56 retitled this revision from [VP] Pass fast math flags to VP SDNodes. to [SelectionDAGBuilder] Pass fast math flags to most of VP SDNodes..May 15 2022, 5:49 AM
fakepaper56 edited the summary of this revision. (Show Details)
fakepaper56 marked an inline comment as done.
fakepaper56 added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7603

Thank your reminder. I fix the bug in the latest update.

craig.topper added inline comments.May 15 2022, 12:51 PM
llvm/test/CodeGen/RISCV/pass-fast-math-flags-sdnode.ll
2

You need REQUIRES: asserts if you use -debug-only. The -debug-only option isn't available on builds without asserts.

craig.topper added inline comments.May 15 2022, 12:55 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7615

Why can't we do?

SDValue Result;
SDNodeFlags SDFlags;
if (auto *FPMO = dyn_cast<FPMathOperator>(&VPIntrin))
  SDFlags.copyFMF(*FPMO);
Result = DAG.getNode(Opcode, DL, VTs, OpValues, SDFlags);
fakepaper56 marked an inline comment as done.May 15 2022, 7:24 PM
fakepaper56 added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7615

getNode with SDVTList does not use SDNodeFlags directly.

SDValue SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL, SDVTList VTList,
                              ArrayRef<SDValue> Ops, const SDNodeFlags Flags) {
  if (VTList.NumVTs == 1) 
    return getNode(Opcode, DL, VTList.VTs[0], Ops);
craig.topper added inline comments.May 15 2022, 7:31 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7615

Can you submit a patch to fix that? It’s a bug.

fakepaper56 edited the summary of this revision. (Show Details)

Add REQUIRES: asserts in pass-fast-math-flags-sdnode.ll.

fakepaper56 marked an inline comment as done.May 15 2022, 8:09 PM
fakepaper56 added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7615

Yeah, I am willing to do that.

After fixing getNode, pass SDNodeFlags to getNode of SDVTList version.

craig.topper added inline comments.May 16 2022, 10:37 AM
llvm/test/CodeGen/RISCV/pass-fast-math-flags-sdnode.ll
2

I didn't think about this before, but is it possible to use -stop-after=finalize-isel and inspect the MIR output instead? That wouldn't require an asserts build.

fakepaper56 added inline comments.May 17 2022, 10:55 PM
llvm/test/CodeGen/RISCV/pass-fast-math-flags-sdnode.ll
2

There is no optimization of VP using fast math flags and I don't know how to make MIR print fast-math flags. The output of llc -mtriple=riscv64 -mattr=+v fa. -stop-after=finalize-isel :
%7:vrnov0 = nofpexcept PseudoVFMUL_VV_M1_MASK %8, %0, %1, $v0, killed %6, 6 /* e64 */, 1, implicit $frm

2

Could we get MIR

craig.topper accepted this revision.May 17 2022, 11:20 PM

LGTM

llvm/test/CodeGen/RISCV/pass-fast-math-flags-sdnode.ll
2

I think we're dropping the fast math flags when we go from ISD::VP* to RISCVISD::*_VL. So my suggestion won't work without a lot more work.

We can use the debug output and REQUIRES: asserts

This revision is now accepted and ready to land.May 17 2022, 11:20 PM
This revision was landed with ongoing or failed builds.May 18 2022, 1:15 AM
This revision was automatically updated to reflect the committed changes.