[llvm-commits] [llvm] r172658 - in /llvm/trunk: include/llvm/Analysis/TargetTransformInfo.h lib/Analysis/TargetTransformInfo.cpp lib/Target/X86/X86TargetTransformInfo.cpp

Renato Golin Linaro renato.golin at linaro.org
Sun Jan 20 04:07:47 PST 2013


Hi Chandler,

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

Comments inline below...


On 20 January 2013 10:22, Chandler Carruth <chandlerc at google.com> wrote:

> +#include "llvm/CodeGen/ValueTypes.h"
>>
>
> 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.
>

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

In that case, it'll probably be better to leave in Codegen, not ADT.


 +struct CostTableEntry {

> +  int ISD;       // instruction ID
>> +  MVT Types[2];  // Types { dest, source }
>> +  unsigned Cost; // ideal cost
>> +};
>>
>
> 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.
>

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.


This is a class with zero public interfaces. That doesn't really make
> sense... What are you actually trying to do here?
>

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


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

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



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

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.

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.

The ways I could go from now, inside Codegen:

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)

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.

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?

cheers,
--renato
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130120/c0b52300/attachment.html>


More information about the llvm-commits mailing list