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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

In D94223#2485149 <https://reviews.llvm.org/D94223#2485149>, @craig.topper wrote:

> In D94223#2484326 <https://reviews.llvm.org/D94223#2484326>, @frasercrmck wrote:
>
>> 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 `CheckOpcodeMatcher`s.  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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94223/new/

https://reviews.llvm.org/D94223