<div dir="ltr">Hi Chandler,<div><br></div><div style>Thanks for your review, I agree it's not good enough. I'll revert the patch and try again. My original implementation involved templates and some of what you see is leftovers of that implementaiton (which I should have cleaned).</div>
<div style><br></div><div style>Comments inline below...</div><div style><br></div><div style><br></div><div class="gmail_extra"><div class="gmail_quote">On 20 January 2013 10:22, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br>
<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 dir="ltr"><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">+#include "llvm/CodeGen/ValueTypes.h"<br>
</blockquote><div><br></div></div><div>No. You cannot include a CodeGen header in an Analysis pass. What's more, none of this is used by clients of the analysis pass; it is entirely an implementation detail, so none of the code belongs in this header.</div>
</div></div></div></div></blockquote><div><br></div><div>If I use templates, I can't just use anonymous namespaces, since the type must be complete at compile time. (this was left over of the original template code).<br>
</div><div style><br></div><div style>In that case, it'll probably be better to leave in Codegen, not ADT.</div><div><br></div><div><br></div><div> <span style="color:rgb(80,0,80)">+struct CostTableEntry {</span></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 dir="ltr"><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">
+  int ISD;       // instruction ID<br>
+  MVT Types[2];  // Types { dest, source }<br>
+  unsigned Cost; // ideal cost<br>
+};<br></blockquote><div><br></div></div><div>Why is this in the interface? none of the public interfaces in the classes below deal with this. None of the clients of them can use it. This is an implementation detail.</div>
</div></div></div></div></blockquote><div><br></div><div style>This is the exposed data type in which the users will create for the class below (CostTable) to iterate. The whole point of this patch is to not make it implementation detail and to make sure users only use that table entry while iterating with CostTables. I could put it inside, like CostTable::Entry.</div>
<div><br></div><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 dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div class="im">
<div><span style="color:rgb(34,34,34)">This is a class with zero public interfaces. That doesn't really make sense... What are you actually trying to do here?</span></div></div></div></div></div></blockquote><div><br>
</div><div style>Reminiscent of the old template code, and serving as common code for the other two, specialized classes (that when we dropped the template, became nothing). That's the main problem, IMO, and one I'll review with more care. Maybe putting it in an anonymous namespace would be enough...</div>
<div><br></div><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 dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div class="im"><div><span style="color:rgb(34,34,34)">That sort of utility should either be a generic ADT (and templated), or it should be in lib/CodeGen as that is where shared implementation logic between the targets typically lives. I don't have strong opinions about which design you want to follow here.</span></div>
</div></div></div></div></blockquote><div><br></div><div><div>I was trying to tie up the data with the utility, so that they don't change independently, and can cope with multiple uses. Something that the previous implementation didn't have. I think ADT makes more sense, but I'm not sure there will be other uses for such a "data type" outside the cost model. Besides, I may be forced to implement this in Codegen, since I need MVT headers (for a templated version, at least).</div>
</div><div><br></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 dir="ltr"><div class="gmail_extra">
<div class="gmail_quote"><div class="im"><div><span style="color:rgb(34,34,34)">Second, inheritance is almost certainly the wrong pattern for sharing the logic between the 2-type and 1-type variants. I suspect you want a template, but I'm not certain. I suggest you revert this code and take another stab at it?</span><br>
</div></div></div></div></div></blockquote><div><br></div><div>My original design had the code templated by number of types (since the type list size has to be known at compile time) and had typedefs for the most common, but there were some reviews against it.</div>
<div><br></div><div>Also, I can't have the code templated by table size because that would create the necessity of updating the lookup size every time the table grows. In this case, the usage is not the same as SamllVector, since the whole idea of the tables is to make it easier to change.</div>
<div><br></div></div><div style>The ways I could go from now, inside Codegen:</div><div style><br></div><div style>1. Have a generic CostTable<types> { Entry { } } with all methods templated by type size and some possible typedefs to help implementation. This was the original implementation, and allows multiple-sized type tables. (not sure this is really necessary)</div>
<div style><br></div><div style>2. Have CostTableEntry on its own, CostTable { } on anonymous namespace, and the two derived classes as the only public utilities. This is a derivation from the current implementation and is somewhat disconnected.<br>
</div><div style><br></div><div style>Either way, the main idea was to unite the two previous cost tables into one, leaving no gap for implementation detail on the lookup. I prefer the first one, but am not against the second. Do you have any other suggestions?</div>
<div style><br></div><div style>cheers,</div><div style>--renato</div></div></div>