[llvm-commits] Patch: Pattern matching for intrinsics

[Chandler Carruth]

On Wed, Dec 12, 2012 at 1:53 PM, Chandler Carruth <chandlerc at google.com>wrote:

> On Wed, Dec 12, 2012 at 11:16 AM, Michael Ilseman <milseman at apple.com>wrote:
>
>> Here's some new patches. There's a little bit of dirtiness due to not
>> having variadic templates, but for the most part it's much better. I also
>> go rid of the artificial duplication and classification from before. Thanks
>> for all the suggestions, this is looking cleaner now!
>>
>
> Awesome!
>
>
>> Patch 1: m_CombineOr and m_CombineAnd pattern combinators
>>
>
> Patch looks good... Some day, I would love to have m_Any and m_All instead
> (and they be variadic) but yea, too gross w/o variadics...
>
>
>> Patch 2: Pattern matching code for intrinsic
>>
>
> Looking at this one now...
>

My only real issue here is this:

+template<typename Opnd_t>
+struct Operand_match {
+  unsigned OpI;
+  Opnd_t Val;
+  Operand_match(unsigned OpIdx, const Opnd_t &V) : OpI(OpIdx), Val(V) { }
+
+  template<typename OpTy>
+  bool match(OpTy *V) {
+    if (CallInst *CI = dyn_cast<CallInst>(V))
+      return Val.match(CI->getArgOperand(OpI));
+    if (User *U = dyn_cast<User>(V))
+      return Val.match(U->getOperand(OpI));
+    return false;
+  }
+};

I worry that this is too generalized in a sense. I would just make an
Argument_match that only works on arguments, and not for generic operands.

Also, it should probably handle InvokeInst as well as CallInst. Would it be
possible to define this in terms of a CallSite?


>
>
>>
>> Provides m_Operand that allows matching against a Value's specified
>> operand. Provides m_Intrinsic pattern that can be templatized over the
>> intrinsic id and bind/match operands similarly to other pattern matchers.
>> Implementations provided for 0 to 4 arguments, though it's very simple to
>> extend for more. Also provides example template specialization for bswap
>> (m_BSwap) and example of code cleanup for its use.
>>
>>
>>
>>
>>
>>
>> On Dec 10, 2012, at 8:55 PM, Chandler Carruth <chandlerc at google.com>
>> wrote:
>>
>> On Mon, Dec 10, 2012 at 7:33 PM, Michael Ilseman <milseman at apple.com>wrote:
>>
>>>
>>> On Dec 10, 2012, at 4:53 PM, Chandler Carruth <chandlerc at google.com>
>>> wrote:
>>>
>>> Hey Michael,
>>>
>>> On Dec 7, 2012, at 1:40 PM, Michael Ilseman <milseman at apple.com> wrote:
>>>>
>>>> > Below is a patch to add some pattern matching support for intrinsics
>>>> to PatternMatch.h:
>>>> >
>>>> >
>>>> > Pattern matching code for intrinsics.
>>>> >
>>>> > Provides m_Intrinsic pattern that can be templatized over the
>>>> intrinsic id and bind/match operands similarly to other pattern matchers.
>>>> Also provides example template specialization for bswap (m_BSwap) and
>>>> example of code cleanup for its use. One motiviation for this is to ease
>>>> upcoming peephole fast-math optimizations which may want to inspect into
>>>> built-in intrinsics.
>>>>
>>>
>>> I really like the idea of intrinsic matchers. I think the current
>>> matching patterns need some tweaking though...
>>>
>>> First, for the baseline matching: I would make this pattern able to
>>> match a reasonable number of arguments. I think my ideal syntax would be:
>>>
>>>   m_Intrinsic(Intrinsic::foo, m_Args(m_Foo(), m_Bar(), m_Baz(), ...))
>>>
>>> I'm not sure if having the extra 'm_Args' layer is too much, we could
>>> fold them directly into the top level matcher.
>>>
>>>
>>> I like the suggestion. The multi-argument top level matcher is what's
>>> currently present in my patches, though it's miserably messy. Without
>>> variadic templates, there might have to be a little mess, but I can
>>> probably have a single Intrinsic_match struct that's parameterized over the
>>> operand index.
>>>
>>> I think you can do this for as many arguments as we would ever need in a
>>> pattern matcher by just providing default template arguments for them, and
>>> have, say, 8 of them.
>>>
>>>
>>> Yes, I had implemented m_Intrinsic from 0 to 2 arguments, but there's no
>>> reason not to have more, especially with some cleanup and refactoring of
>>> the structs.
>>>
>>>
>>> I think the only other abstractions which should exist at the matcher
>>> level should be based on the synthetic classes in IntrinsicInst.h. In
>>> particular, I think Unary and Binary aren't good abstractions for intrinsic
>>> calls. And we shouldn't need them if we provide generic argument matching
>>> as above?
>>>
>>>
>>> Unary and Binary were not to be used by the user, but were for
>>> implementation mechanics only. They'll also disappear when I implement your
>>> suggestions above. Given that, are there uses for the synthetic classes?
>>> Would there be a user that would want to e.g. match any non-memread
>>> intrinsic, but not care which one in particular they're matching? Or do you
>>> mean the subclasses of IntrinsicInst?
>>>
>>
>> I mean the particular subclasses.
>>
>>
>>>  If so, then I'd feel more comfortable with someone who actually uses
>>> those adding in special cases, as I'm not familiar with the ways or reasons
>>> that you would want to distinguish them.
>>>
>>
>> I agree. More importantly, we should add them when we have some concrete
>> code to clean up using them. =]
>>
>> An interesting question is whether it's worth adding m_Bswap(...) and
>> other aliases for m_Intrinsic(Intrinsic::bswap, ...)? I don't have any
>> really good insight into the frequency with which you'll need to match
>> particular intrinsics, or when the best cutoff point is for a custom
>> matcher... As a strawman heuristic for choosing when to add a custom
>> matcher, how about adding one for any intrinsic in the langref we match in
>> more than one place? If you anticipate adding lots of matching rules for
>> target-specific intrinsics, I'm not opposed to adding matchers even for
>> those. I would just commit new aliases as they are convenient, they have no
>> real design cost.
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121212/4e1a6d04/attachment.html>