Page MenuHomePhabricator

[DAGCombiner][VP] Fold zero-length or false-masked VP ops
ClosedPublic

Authored by frasercrmck on Sep 2 2021, 5:06 AM.

Details

Summary

This patch adds a generic DAGCombine for vector-predicated (VP) nodes.
Those for which we can determine that no vector element is active can be
replaced by either undef or, for reductions, the start value.

This is tested rather trivially at the IR level, where it's possible
that we want to teach instcombine to perform this optimization.

However, we can also see the zero-evl case arise during SelectionDAG
legalization, when wide VP operations can be split into two and the
upper operation emerges as trivially false.

It's possible that we could perform this optimization "proactively"
(both on legal vectors and before splitting) and reduce the width of an
operation and insert it into a larger undef vector:

v8i32 vp_add x, y, mask, 4
->
v8i32 insert_subvector (v8i32 undef), (v4i32 vp_add xsub, ysub, mask, 4), i32 0

This is somewhat analogous to similar vector narrow/widening
optimizations, but it's unclear at this point whether that's beneficial
to do this for VP ops for any/all targets.

Diff Detail

Event Timeline

frasercrmck created this revision.Sep 2 2021, 5:06 AM
frasercrmck requested review of this revision.Sep 2 2021, 5:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2021, 5:06 AM
frasercrmck added a project: Restricted Project.Sep 2 2021, 5:07 AM
simoll added a comment.Sep 3 2021, 1:53 AM

This makes sense. +1 for also doing the corresponding IR optimization in InstSimplify.

I am not so sure about pro-active type narrowing (introducing insert_subvector nodes), at least not for all targets: we take VP nodes pretty much raw in the VE backend.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
22063

You could use the property macros in include/llvm/IR/VPIntrinsics.def for this intead.
Eg, define ISD::isVPBinaryOp(ISD) amd ISD::isVPReductionOP(ISD) and use them here.

22083

cast<MemSDNode> and handle all mem ops generically?

MemSDNode::getChain() gives you the chain.

MemSDNode::readMem() tells you whether we need an UNDEF.

This should automatically handle the upcoming strided_load/store.

  • remove switch
  • use binary-op and reduction helpers
  • use MemSDNode to simplify load, store, etc.
frasercrmck marked 2 inline comments as done.Sep 6 2021, 5:15 AM
frasercrmck added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
22063

All good ideas, thank you. I've added those now. I slightly changed the macros from HANDLE_ to PROPERTY since they're not currently being used to "handle" anything: they just flag an opcode as having a certain property.

RKSimon added inline comments.Sep 7 2021, 10:32 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1744

Should we have a #undef BEGIN_REGISTER_VP_SDNODE after the include?

craig.topper added inline comments.Sep 7 2021, 10:47 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1744

Isn't it undefed at the end of VPIntrinsics.def?

RKSimon added inline comments.Sep 7 2021, 1:00 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
1744

So it does - sorry about that :)

LGTM. We should cleanup the VPIntrinsics.def file some time .. originally every property was attached to some entity (intrinsic and/or sdnode) by placing the property macro in the scope of the entity. Now, we are seeing the ID being repeated in the property macros, making it independent of the scope. Let's discuss this on the call. This should not hold back the patch.

frasercrmck marked an inline comment as done.Fri, Sep 24, 8:03 AM

LGTM. We should cleanup the VPIntrinsics.def file some time .. originally every property was attached to some entity (intrinsic and/or sdnode) by placing the property macro in the scope of the entity. Now, we are seeing the ID being repeated in the property macros, making it independent of the scope. Let's discuss this on the call. This should not hold back the patch.

Makes sense, let's discuss.

I'll need someone's official acceptance before I can merge this patch. I'll also get round to doing the same thing in InstSimplify some time.

This revision is now accepted and ready to land.Fri, Sep 24, 9:41 AM
This revision was landed with ongoing or failed builds.Mon, Sep 27, 3:40 AM
This revision was automatically updated to reflect the committed changes.