[llvm-commits] Cost Table, take 2

[Nadav Rotem]

Renato spent way too much time on this. He is an ARM expert, but in the last week he spent his evenings writing and re-writing and re-writing different table implementations instead of focusing on what he initially planned to do. I am perfectly happy with any one of the solutions that Renato proposed. The LLVM-way is to get something in and refactor it later if needed. Chandler refactored the TargetTransformInfo layer and it is now awesome. If we need to refactor the cost-tables later then we can do it. Renato, please choose whatever implementation that you think is right. Chandler, please let us know if you see any major fouls or coding standard violations. Renato, I am grateful for the work that you did and I look forward to seeing the ARM cost model.

Thanks,
Nadav

On Jan 24, 2013, at 1:32 AM, Chandler Carruth <chandlerc at google.com> wrote:

> On Thu, Jan 24, 2013 at 1:01 AM, Renato Golin Linaro <renato.golin at linaro.org> wrote:
> On 24 January 2013 08:45, Chandler Carruth <chandlerc at google.com> wrote:
> At this point, I'm not really seeing the benefit of this patch.
> 
> As usual, the patch is changing its original intent from reviews, so it may well be a mess because it's trying to accommodate everyone's ideas.
> 
> My original proposal was to *just* move the two cost structures out of x86 code, so that the ARM code could use it, but many constraints appeared, including making it a full ADT type. So, it's bigger than it should because it may be expecting to work on areas it wasn't originally planned.
> 
> I'm sorry. I wasn't trying to push toward making it more generic in my comments. I wasn't aware of the previous iterations, thought you *wanted* it to be a generic ADT construct, and was trying to give ideas about how to go about that in the abstract... I didn't mean to seem like I was pushing for that particular solution.
> 
> 
> 1) A TargetCostTable class template in include/llvm/Target which accepts a T type parameter that can be either MVT, std::pair<MVT, MVT>, or std::tuple<MVT, MVT, ...>. Then the ISD and the cost value can be concrete things.
> 
> Yet another possible place, yet another slightly different implementation detail... We're going in circles and I won't accommodate every one's designs because they're all incompatible. 
> 
> The efforts in changing this have already outweighed its costs 10-fold. I'm tempted to just leaving as it is and move the two tables and their two functions with generic names to lib/Target and call it a day.
> 
> I'm sorry you're frustrated. I'm not trying to slow things down or make you accomodate everyone's wishes, I'm just trying to find a good abstraction and design for this code. Especially when it's going into a really low-level library like ADT or Support there is a really high bar in terms of design review...
> 
> I wasn't really following the original review thread, so I can't comment about the other constraints or other directions. As an outsider to the discussion (and to the ARM backend entirely) it *seems* like your original proposal makes plenty of sense, but I just don't have the context there. Since Nadav is working on both of these cost models, maybe he can pick the design direction that makes the most sense?

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130124/8a2fae10/attachment.html>