Remove support for multi-op OpDescriptions.

[tsymalla]

OpSet and OpMap only support querying of single-opcode OpDescriptions. This change modifies OpDescription to only allow storing a single operation, while OpSet becomes the first-class citizen for storing operand sets.
This change also adds some missing Visitor implementations to make working with the Visitor and the OpSet more pleasant.

[nhaehnle]

That was fast, thanks!

[nhaehnle]

This is a regression in a sense, and it would be nice if we could avoid it. What if the VisitorBuilder always used an OpSet, even for singletons?

Basically, what I'm thinking of is to just get rid of VisitorKey entirely and always using OpSet. Or at least removing the OpDescription case from the VisitorKey (and keeping the set and intrinsic cases).

This technically makes some of the VisitorBuilder code less efficient, but:

• By design, visitor building should happen only once, so a small regression isn't too bad
• All the OpSet::get implementations should still be fast since they should return references to static local variables
• Fixing this particular regression and cleaning up the code slightly is a benefit that I'd say outweighs a minor performance different during visitor building

(That would also mean removing OpDescription::get ... actually, I wonder if perhaps we end up removing OpDescription altogether in the end? I haven't fully thought this through to the end.)

[tsymalla]

Theoretically, that would be possible. Maybe let us discuss a mid-term plan for that offline, since I regard OpDescription as a useful layer of abstraction right now.

[tsymalla]

Thinking a bit more, I doubt an intermediate solution makes sense. The issue here is the group of operations, namingly UnaryInstruction and BinaryOperator, as currently implemented by the getClass specialization approach.

If we want to register a BinaryOperator case to the Visitor and make it forward to the actual ::get specialization of the OpSet, this requires a not-so-beautiful template overload for the parameter pack case:

template <typename OpT> static const OpSet &get();

  // Construct an OpSet from a set of dialect ops, given as template
  // arguments.
  template <typename... OpTs, std::size_t Count = sizeof...(OpTs), std::enable_if_t<(Count > 1), bool> = true> static const OpSet get() {
    static OpSet set;
    (... && appendT<OpTs>(set));
    return set;
  }

Otherwise, the compiler will complain about ambiguity, in which he is correct. Even though we have a single-argument template overload of ::get, we can make it work, but that requires the dialect generator to generate OpSet overloads instead of OpDescription overloads, because without, nothing else will work.
That in turn will require us to modify OpMap to accept OpSets instead of OpDescriptions. We cannot make use of the getClass approach without adding a specific VisitorBuilder::addClass method that only forwards to the specific OpSet::get specialization.

Of course we can make it work, but at this point, we have several choices:

• Introduce OpSet support as first-class citizen, removing support for BinaryOperator and UnaryInstruction
• Fully remove support for OpDescription::get as part of this PR and do all of the required changes (I have a half-baked version on my disk)
• Integrate OpSet in the Visitor and only add specific OpSet::get specializations next to the existing OpDescription overloads
• Leave it in the current state as of this PR, but accept the regression

I tend to fully drop support for OpDescription::get as part of this PR.
We will of course require special overloads of the VisitorBuilder::add methods to prefer single-argument template invocations, so we can do forwarding with the specific OpT as argument type (OpT &Op instead of llvm::Instruction &I), but maybe that will be a bit cleaner.

[tsymalla]

https://github.com/tsymalla/llvm-dialects/tree/remove_opdescription_multiops_add_opset includes the changes, showing what I mean.