This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Extend immAll(Ones|Zeros)V to handle ISD::SPLAT_VECTOR
ClosedPublic

Authored by frasercrmck on Jan 7 2021, 3:50 AM.

Details

Summary

The TableGen immAllOnesV and immAllZerosV helpers implicitly wrapped the
ISD::isBuildVectorAll(Ones|Zeros) helper functions. This was inhibiting
their use for targets such as RISC-V which use ISD::SPLAT_VECTOR. In
particular, RISC-V had to define its own 'vnot' fragment.

In order to extend the scope of these nodes to include support for
ISD::SPLAT_VECTOR, two new ISD predicate functions have been introduced:
ISD::isConstantSplatVectorAll(Ones|Zeros). These effectively supersede
the older "isBuildVector" predicates, which are now simple wrappers for
the new functions. They pass a defaulted boolean toggle which preserves
the old behaviour. It is hoped that in time all call-sites can be ported
to the "isConstantSplatVector" functions.

While the use of ISD::isBuildVectorAll(Ones|Zeros) has not changed, the
behaviour of the TableGen immAll(Ones|Zeros)V has. To test the new
functionality, the custom RISC-V TableGen fragment has been removed and
replaced with the built-in 'vnot'. To test their use as pattern-roots, two
splat patterns have been updated accordingly.

Diff Detail

Unit TestsFailed

Event Timeline

frasercrmck created this revision.Jan 7 2021, 3:50 AM
frasercrmck requested review of this revision.Jan 7 2021, 3:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2021, 3:50 AM

This seems to be missing test coverage.

  • update documentation

This seems to be missing test coverage.

The functionality is tested in part by replacing riscv_m_vnot with vnot and having the patterns still match. I've just identified that there are two further RVV patterns which can potentially be updated to use immAllOnesV and immAllZeroV and will try that out accordingly.

But I would gladly add more testing (for more targets) if someone can help me find a way in.

Ah. I've encountered a hiccup when fixing this up so SPLAT_VECTOR can be used as a root of immAllOnesV and immAllZerosV, e.g.

def : Pat<(mti.Mask immAllOnesV),
          (!cast<Instruction>("PseudoVMSET_M_"#mti.BX) VLMax, mti.SEW)>;

@craig.topper, do you know if it's possible to extend the following code to allow SPLAT_VECTOR? My naive attempts are telling me that you can't just add two independent CheckOpcodeMatchers. I couldn't get SwitchOpcodeMatcher or ScopeMatcher (which seemed to optimize to SwitchOpcodeMatcher) to work. I'm suspecting that it can't place the emission code in the right place when I do that. We don't need a new kind of OPC, do we?

// If this is the root of the dag we're matching, we emit a redundant opcode
// check to ensure that this gets folded into the normal top-level
// OpcodeSwitch.
if (N == Pattern.getSrcPattern()) {
  const SDNodeInfo &NI = CGP.getSDNodeInfo(CGP.getSDNodeNamed("build_vector"));
  AddMatcher(new CheckOpcodeMatcher(NI));
}
return AddMatcher(new CheckImmAllOnesVMatcher());

Ah. I've encountered a hiccup when fixing this up so SPLAT_VECTOR can be used as a root of immAllOnesV and immAllZerosV, e.g.

def : Pat<(mti.Mask immAllOnesV),
          (!cast<Instruction>("PseudoVMSET_M_"#mti.BX) VLMax, mti.SEW)>;

@craig.topper, do you know if it's possible to extend the following code to allow SPLAT_VECTOR? My naive attempts are telling me that you can't just add two independent CheckOpcodeMatchers. I couldn't get SwitchOpcodeMatcher or ScopeMatcher (which seemed to optimize to SwitchOpcodeMatcher) to work. I'm suspecting that it can't place the emission code in the right place when I do that. We don't need a new kind of OPC, do we?

// If this is the root of the dag we're matching, we emit a redundant opcode
// check to ensure that this gets folded into the normal top-level
// OpcodeSwitch.
if (N == Pattern.getSrcPattern()) {
  const SDNodeInfo &NI = CGP.getSDNodeInfo(CGP.getSDNodeNamed("build_vector"));
  AddMatcher(new CheckOpcodeMatcher(NI));
}
return AddMatcher(new CheckImmAllOnesVMatcher());

I think AddMatcher has some hidden state that keeps track of the last node added to build a linked list. You would need to put two entries into the PatternMatchers vector in DAGISelEmitter::run. I think we might be able to special case immAllZerosV/immAllOnesV at the top of MatcherGen::EmitMatcherCode where complex pattern is handled. We can emit a build_vector CheckOpcodeMatcher for variant 0 for build_vector and splat_vector for variant 1. And return true for variant >= 2. Then we remove the special case for "if (N == Pattern.getSrcPattern()) {" where these are handled in EmitLeafMatchCode.

Ah. I've encountered a hiccup when fixing this up so SPLAT_VECTOR can be used as a root of immAllOnesV and immAllZerosV, e.g.

def : Pat<(mti.Mask immAllOnesV),
          (!cast<Instruction>("PseudoVMSET_M_"#mti.BX) VLMax, mti.SEW)>;

@craig.topper, do you know if it's possible to extend the following code to allow SPLAT_VECTOR? My naive attempts are telling me that you can't just add two independent CheckOpcodeMatchers. I couldn't get SwitchOpcodeMatcher or ScopeMatcher (which seemed to optimize to SwitchOpcodeMatcher) to work. I'm suspecting that it can't place the emission code in the right place when I do that. We don't need a new kind of OPC, do we?

// If this is the root of the dag we're matching, we emit a redundant opcode
// check to ensure that this gets folded into the normal top-level
// OpcodeSwitch.
if (N == Pattern.getSrcPattern()) {
  const SDNodeInfo &NI = CGP.getSDNodeInfo(CGP.getSDNodeNamed("build_vector"));
  AddMatcher(new CheckOpcodeMatcher(NI));
}
return AddMatcher(new CheckImmAllOnesVMatcher());

I think AddMatcher has some hidden state that keeps track of the last node added to build a linked list. You would need to put two entries into the PatternMatchers vector in DAGISelEmitter::run. I think we might be able to special case immAllZerosV/immAllOnesV at the top of MatcherGen::EmitMatcherCode where complex pattern is handled. We can emit a build_vector CheckOpcodeMatcher for variant 0 for build_vector and splat_vector for variant 1. And return true for variant >= 2. Then we remove the special case for "if (N == Pattern.getSrcPattern()) {" where these are handled in EmitLeafMatchCode.

I tried this out and got it to work, but it does add redundant table entries to other targets that only support build_vector for their all zeros/ones. It was about 635 extra bytes on X86. For a given target and VT it should always be build_vector or splat_vector, not both. So I'm not sure if we should add the unneeded entries or just document that it doesn't work for splat_vector at the root node and keep the explicit splat_vector match on RISCV. If we keep the splat_vector explicit, we should change it to only look at bit 0 of the immediate.

craig.topper added a comment.EditedJan 7 2021, 1:30 PM

New idea, just pick opcode based on the node value type being a scalable vector.

diff --git a/llvm/utils/TableGen/DAGISelMatcherGen.cpp b/llvm/utils/TableGen/DAGISelMatcherGen.cpp
index 792bf17..ad99304 100644
--- a/llvm/utils/TableGen/DAGISelMatcherGen.cpp
+++ b/llvm/utils/TableGen/DAGISelMatcherGen.cpp
@@ -282,7 +282,9 @@ void MatcherGen::EmitLeafMatchCode(const TreePatternNode *N) {
     // check to ensure that this gets folded into the normal top-level
     // OpcodeSwitch.
     if (N == Pattern.getSrcPattern()) {
-      const SDNodeInfo &NI = CGP.getSDNodeInfo(CGP.getSDNodeNamed("build_vector"));
+      MVT VT = N->getSimpleType(0);
+      std::string Name = VT.isScalableVector() ? "splat_vector" : "build_vector";
+      const SDNodeInfo &NI = CGP.getSDNodeInfo(CGP.getSDNodeNamed(Name));
       AddMatcher(new CheckOpcodeMatcher(NI));
     }
     return AddMatcher(new CheckImmAllOnesVMatcher());
@@ -292,7 +294,9 @@ void MatcherGen::EmitLeafMatchCode(const TreePatternNode *N) {
     // check to ensure that this gets folded into the normal top-level
     // OpcodeSwitch.
     if (N == Pattern.getSrcPattern()) {
-      const SDNodeInfo &NI = CGP.getSDNodeInfo(CGP.getSDNodeNamed("build_vector"));
+      MVT VT = N->getSimpleType(0);
+      std::string Name = VT.isScalableVector() ? "splat_vector" : "build_vector";
+      const SDNodeInfo &NI = CGP.getSDNodeInfo(CGP.getSDNodeNamed(Name));
       AddMatcher(new CheckOpcodeMatcher(NI));
     }
     return AddMatcher(new CheckImmAllZerosVMatcher());

Note, I had to use std::string because getSDNodeNamed takes a const std::string &. I'll investigate changing it to StringRef. We only ever call it with string literals so it's dumb to force a std::string temporary.

New idea, just pick opcode based on the node value type being a scalable vector.

Note, I had to use std::string because getSDNodeNamed takes a const std::string &. I'll investigate changing it to StringRef. We only ever call it with string literals so it's dumb to force a std::string temporary.

Oh, neat! That hadn't occurred to me; I wasn't in the TableGen headspace. I was thinking last night that it's not ideal to add "redundant" SPLAT_VECTOR code to other targets' tables, so I like that your idea avoids that. I would prefer if it could act identically on both opcodes (and it would make the RVV fixed-length implementation simpler if we could assume splats everywhere) but I think the fixed-length<->BUILD_VECTOR relationship goes quite deep for obvious reasons so it's probably not achievable.

I'll add your idea to this patch if that's alright.

  • fix immAll(Ones|Zeros)V when used as root
  • use that in RVV patterns
frasercrmck edited the summary of this revision. (Show Details)Jan 8 2021, 2:25 AM
craig.topper accepted this revision.Jan 8 2021, 9:56 AM

LGTM

llvm/utils/TableGen/DAGISelMatcherGen.cpp
286

This can be a StringRef now. I fixed getSDNodeNamed yesterday.

300

Same here

This revision is now accepted and ready to land.Jan 8 2021, 9:56 AM
frasercrmck edited the summary of this revision. (Show Details)

rebase on main
change std::string to StringRef

frasercrmck marked 2 inline comments as done.Jan 9 2021, 4:00 AM