<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><div dir="ltr">On Wed, Dec 12, 2012 at 1:53 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank" class="cremed">chandlerc@google.com</a>></span> wrote:<br>
<div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="font-family:arial,helvetica,sans-serif;font-size:10pt">
<div dir="ltr"><div class="im">On Wed, Dec 12, 2012 at 11:16 AM, Michael Ilseman <span dir="ltr"><<a href="mailto:milseman@apple.com" target="_blank" class="cremed">milseman@apple.com</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><div class="im"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word">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!</div>
</blockquote><div><br></div></div><div>Awesome!</div><div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><br></div>
<div>Patch 1: m_CombineOr and m_CombineAnd pattern combinators</div></div></blockquote><div><br></div></div><div>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...</div>
<div class="im">
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br>
</div><div>Patch 2: Pattern matching code for intrinsic</div>
</div></blockquote><div><br></div></div><div>Looking at this one now...</div></div></div></div></div></blockquote><div><br></div><div style>My only real issue here is this:</div><div style><br></div><div style><div>+template<typename Opnd_t></div>
<div>+struct Operand_match {</div><div>+ unsigned OpI;</div><div>+ Opnd_t Val;</div><div>+ Operand_match(unsigned OpIdx, const Opnd_t &V) : OpI(OpIdx), Val(V) { }</div><div>+</div><div>+ template<typename OpTy></div>
<div>+ bool match(OpTy *V) {</div><div>+ if (CallInst *CI = dyn_cast<CallInst>(V))</div><div>+ return Val.match(CI->getArgOperand(OpI));</div><div>+ if (User *U = dyn_cast<User>(V))</div><div>+ return Val.match(U->getOperand(OpI));</div>
<div>+ return false;</div><div>+ }</div><div>+};</div><div><br></div><div style>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.</div>
<div style><br></div><div style>Also, it should probably handle InvokeInst as well as CallInst. Would it be possible to define this in terms of a CallSite? </div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="font-family:arial,helvetica,sans-serif;font-size:10pt"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im"><div style="word-wrap:break-word">
<div><br></div><div>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.</div>
<div><br></div><div><div></div></div></div><br><div style="word-wrap:break-word"><div><div><br><div></div></div></div></div>
<br></div><div style="word-wrap:break-word"><div><div><div></div><div><br><div><br><div><div class="im"><div>On Dec 10, 2012, at 8:55 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank" class="cremed">chandlerc@google.com</a>> wrote:</div>
<br></div><blockquote type="cite"><div style="font-family:arial,helvetica,sans-serif;font-size:10pt"><div dir="ltr"><div class="im">On Mon, Dec 10, 2012 at 7:33 PM, Michael Ilseman <span dir="ltr"><<a href="mailto:milseman@apple.com" target="_blank" class="cremed">milseman@apple.com</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word">
<br><div><div><div class="im"><div>On Dec 10, 2012, at 4:53 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank" class="cremed">chandlerc@google.com</a>> wrote:</div>
<br></div><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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div class="im">On Dec 7, 2012, at 1:40 PM, Michael Ilseman <<a href="mailto:milseman@apple.com" target="_blank" class="cremed">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></div><div class="im">
> Pattern matching code for intrinsics.<br>
><br></div><div class="im">
> 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 class="im"><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></div></blockquote><div><br></div></div><div class="im"><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><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><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></div><div class="im"><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></div></blockquote><div class="im"><div><br></div><div>I mean the particular subclasses.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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></div><br></div><div class="im"><div class="gmail_extra">I agree. More importantly, we should add them when we have some concrete code to clean up using them. =]</div><div class="gmail_extra"><br></div><div class="gmail_extra">
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></div>
</blockquote></div><br></div></div></div></div></div>
<br></blockquote></div><br></div></div></div>
</blockquote></div><br></div></div></div>