[llvm-commits] Patch: Pattern matching for intrinsics

[Michael Ilseman]

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.
> 
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121212/1d5746f8/attachment.html>