<div dir="ltr"><div class="gmail_default" style>On Fri, Jan 4, 2013 at 11:57 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"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im"><br>
On Jan 4, 2013, at 11:05 AM, Shuxin Yang <<a href="mailto:shuxin.llvm@gmail.com" class="cremed">shuxin.llvm@gmail.com</a>> wrote:<br>
<br>
> I personally think it is more natural to define isFNeg() as a member function of class BinaryOperator.<br>
> It is in many ways similar to BinaryOperator::isCompoundAssignmentOp(), .., BinaryOperator::isShiftAssignOp() etc.<br>
> How come it sounds odd to you.<br>
<br>
</div>I think Duncan was saying that all those methods feel a bit antiquated. In a separate patch, it might be good to remove these, but I'll let someone who has better historical context make that call. I don't know what's considered modern LLVM style for those checks, but I think it might be to use PatternMatch.h.</blockquote>
<div><br></div><div style>Without trying to address this specific patch (I've not looked), I'll try to answer the general question of how these APIs should be structured:</div><div style><br></div><div style>I think it is a good thing to have obvious predicates on instruction classes, but I think we should carefully limit the proliferation of subclass predicates in the parent class APIs. To be clear, this last category consists of any predicate like so:</div>
<div style><br></div><div style>class Derived;</div><div style>class Base {</div><div style>public:</div><div style>  bool isDerivedProperty() { if (Derived *D = dyn_cast<Derived>(this)) return D->isProperty(); else return false; }</div>
<div style>};</div><div style><br></div><div style>The only time this pattern makes sense to me is when there is a tremendously large number of places which boil down to the dyn_cast + method test, and there is no other use of the derived typed pointer, and a reasonable number of them occur within large compound predicates making the 2-step test significantly more code than the base class predicate would be.</div>
<div style><br></div><div style>The best examples of this are in the Type class, where there really are systematic patterns that we can make shorter this way.</div><div style><br></div><div style><br></div><div style>I think we should move completely away from methods which are entirely equivalent to { return isa<Derived>(this); } and instead just write the 'isa' pattern.</div>
<div style><br></div><div style><br></div><div style>Finally, there is a reason to have *both* a predicate on a particular instruction class and a pattern match interface: the pattern matching interfaces *compose*! That is their key power, and the primary reason we should be expanding the pattern matching support for the IR patterns that optimizations need to match (yes, it is well named for once). Personally, I suspect there should be nearly a 1:1 correspondence between predicate methods on instruction classes and pattern matching utilities. For example, BranchInst can be both conditional and unconditional, and we need pattern matching to support both forms.</div>
<div style><br></div><div style><br></div><div style>Now, BinaryOperator is quite special because the specific binary operators don't get their own instruction classes. I think this is generally a good thing to limit the interface creep of the IR, but at some point it might be reasonable to introduce subclasses for particular instructions where there are a large number of useful predicates which are only relevant to a *single* operation. I don't think we're there yet though. As long as the predicate methods on BinaryOperator make sense for more than one operation, I think they're totally reasonable.</div>
<div style><br></div><div style>At most, I'm seeing some early indications that it might me nice to have broad categories such as FPBinaryOperator that could help organize those predicates. We can add that subclass lazily though by keeping the predicates which would sink into such a class clearly organized, and making them assert fail (rather than just return false) if called on an inappropriate operation.</div>
</div></div></div>