[llvm-commits] Patch: Pattern matching for intrinsics

Wed Dec 12 17:41:56 PST 2012

On phone but just go for it. =] we can clean up anything left post commit.
On Dec 12, 2012 4:05 PM, "Michael Ilseman" <milseman at apple.com> wrote:

> New round of patches. Hopefully I'm converging on something reasonable.
>
>
> Patch 1 is unchanged
>
>
>
> Patch 2: Pattern matching code for intrinsics.
>
> Provides m_Argument that allows matching against a CallSite's specified
> argument. Provides m_Intrinsic pattern that can be templatized over the
> intrinsic id and bind/match arguments 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 12, 2012, at 3:46 PM, Michael Ilseman <milseman at apple.com> wrote:
>
>
> On Dec 12, 2012, at 2:03 PM, Chandler Carruth <chandlerc at google.com>
> wrote:
>
> 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.
>
>
> Could you elaborate? I'm having a hard time understanding when something
> is too general or too specific here. I can certainly change it to
> Argument_match if that's cleaner.
>
>
>
> Also, it should probably handle InvokeInst as well as CallInst. Would it
> be possible to define this in terms of a CallSite?
>
>
>
> Cool, I wasn't aware of CallSite. I'll definitely use it.
>
>
>>
>>>
>>> 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.
>>>
>>>
>>>
>>>
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121212/095e0fb6/attachment.html>