[llvm-commits] Patch: Pattern matching for intrinsics

[Chandler Carruth]

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/20121210/ce15eb90/attachment.html>