<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><div dir="ltr">On Mon, Dec 10, 2012 at 7:33 PM, Michael Ilseman <span dir="ltr"><<a href="mailto:milseman@apple.com" target="_blank">milseman@apple.com</a>></span> wrote:<br>
<div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div class="im"><div>On Dec 10, 2012, at 4:53 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> wrote:</div>
<br><blockquote type="cite"><div style="font-family:arial,helvetica,sans-serif;font-size:10pt"><div dir="ltr">Hey Michael,<div><br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div><div>On Dec 7, 2012, at 1:40 PM, Michael Ilseman <<a href="mailto:milseman@apple.com" target="_blank">milseman@apple.com</a>> wrote:<br>
<br>
> Below is a patch to add some pattern matching support for intrinsics to PatternMatch.h:<br>
><br>
><br>
> Pattern matching code for intrinsics.<br>
><br>
> 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.<br>

</div></div></blockquote><div><br></div><div>I really like the idea of intrinsic matchers. I think the current matching patterns need some tweaking though...</div><div><br></div><div>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:</div>

<div><br></div><div>  m_Intrinsic(Intrinsic::foo, m_Args(m_Foo(), m_Bar(), m_Baz(), ...))</div><div><br></div><div>I'm not sure if having the extra 'm_Args' layer is too much, we could fold them directly into the top level matcher.</div>

<div><br></div></div></div></div></div></div></blockquote><div><br></div></div><div>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.</div>
<div class="im"><br><blockquote type="cite"><div style="font-family:arial,helvetica,sans-serif;font-size:10pt"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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.</div>
<div><br></div></div></div></div></div></blockquote><div><br></div></div><div>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.</div>
<div class="im"><br><blockquote type="cite"><div style="font-family:arial,helvetica,sans-serif;font-size:10pt"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>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?</div>

</div></div></div></div>
</blockquote></div></div><br><div>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?</div>
</div></blockquote><div><br></div><div style>I mean the particular subclasses.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">
<div> 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.</div></div></blockquote>
</div><br></div><div class="gmail_extra" style>I agree. More importantly, we should add them when we have some concrete code to clean up using them. =]</div><div class="gmail_extra" style><br></div><div class="gmail_extra" style>
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.</div>
</div></div>