<div dir="ltr">Renato, I know that Nadav gave the OK on this, but this patch is not yet ready for the tree. It has serious layering violations in it, and the design doesn't really make sense yet. It's also not following the coding standards.<div>
<br></div><div>Some detailed comments below to try to help you formulate a new version....<div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 16, 2013 at 1:29 PM, Renato Golin <span dir="ltr"><<a href="mailto:renato.golin@linaro.org" target="_blank" class="cremed">renato.golin@linaro.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: rengolin<br>
Date: Wed Jan 16 15:29:55 2013<br>
New Revision: 172658<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=172658&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=172658&view=rev</a><br>
Log:<br>
Change CostTable model to be global to all targets<br>
<br>
Moving the X86CostTable to a common place, so that other back-ends<br>
can share the code. Also simplifying it a bit and commoning up<br>
tables with one and two types on operations.<br>
<br>
Modified:<br>
llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h<br>
llvm/trunk/lib/Analysis/TargetTransformInfo.cpp<br>
llvm/trunk/lib/Target/X86/X86TargetTransformInfo.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h?rev=172658&r1=172657&r2=172658&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h?rev=172658&r1=172657&r2=172658&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h (original)<br>
+++ llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h Wed Jan 16 15:29:55 2013<br>
@@ -27,6 +27,7 @@<br>
#include "llvm/IR/Type.h"<br>
#include "llvm/Pass.h"<br>
#include "llvm/Support/DataTypes.h"<br>
+#include "llvm/CodeGen/ValueTypes.h"<br></blockquote><div><br></div><div style>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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
+//======================================= COST TABLES ==<br></blockquote><div><br></div><div style>It's just a nit pick, but please don't use new heading styles. We have conventional heading style if you feel you mast have them, but I think they are very rarely useful...</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+/// \brief An entry in a cost table<br>
+///<br>
+/// Use it as a static array and call the CostTable below to<br>
+/// iterate through it and find the elements you're looking for.<br>
+///<br>
+/// Leaving Types with fixed size to avoid complications during<br>
+/// static destruction.<br>
+struct CostTableEntry {<br>
+ int ISD; // instruction ID<br>
+ MVT Types[2]; // Types { dest, source }<br>
+ unsigned Cost; // ideal cost<br>
+};<br></blockquote><div><br></div><div style>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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+/// \brief Cost table, containing one or more costs for different instructions<br>
+///<br>
+/// This class implement the cost table lookup, to simplify<br>
+/// how targets declare their own costs.<br>
+class CostTable {<br>
+ const CostTableEntry *table;<br>
+ const size_t size;<br>
+ const unsigned numTypes;<br></blockquote><div><br></div><div style>Please see the LLVM coding standards. They are very clear and precise about the naming conventions expected. Same comment applies to the methods and constants in this file.</div>
<div style><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+protected:<br>
+ /// Searches for costs on the table<br>
+ unsigned _findCost(int ISD, MVT *Types) const;<br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">+<br>
+ // We don't want to expose a multi-type cost table, since types are not<br>
+ // sequential by nature. If you need more cost table types, implement<br>
+ // them below.<br>
+ CostTable(const CostTableEntry *table, const size_t size, unsigned numTypes);<br>
+<br>
+public:<br>
+ /// Cost Not found while searching<br>
+ static const unsigned COST_NOT_FOUND = -1;<br>
+};<br></blockquote><div><br></div><div style>This is a class with zero public interfaces. That doesn't really make sense... What are you actually trying to do here?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+/// Specialisation for one-type cost table<br>
+class UnaryCostTable : public CostTable {<br>
+public:<br>
+ UnaryCostTable(const CostTableEntry *table, const size_t size);<br>
+ unsigned findCost(int ISD, MVT Type) const;<br>
+};<br></blockquote><div><br></div><div style>This isn't a specialization. To be pedantic, those have to do with templates. It also isn't a subclass or "extending" CostTable in any interesting way: Clients can't use it as a generic CostTable because that doesn't expose any public methods.</div>
<div style><br></div><div><br></div><div style>OK, let's look at how you're using it:</div><div style><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
- static const X86CostTblEntry AVX1CostTable[] = {<br>
- // We don't have to scalarize unsupported ops. We can issue two half-sized<br>
- // operations and we only need to extract the upper YMM half.<br>
- // Two ops + 1 extract + 1 insert = 4.<br>
- { ISD::MUL, MVT::v8i32, 4 },<br>
- { ISD::SUB, MVT::v8i32, 4 },<br>
- { ISD::ADD, MVT::v8i32, 4 },<br>
- { ISD::MUL, MVT::v4i64, 4 },<br>
- { ISD::SUB, MVT::v4i64, 4 },<br>
- { ISD::ADD, MVT::v4i64, 4 },<br>
- };<br>
+ // We don't have to scalarize unsupported ops. We can issue two half-sized<br>
+ // operations and we only need to extract the upper YMM half.<br>
+ // Two ops + 1 extract + 1 insert = 4.<br>
+ static const CostTableEntry AVX1CostTable[] = {<br>
+ { ISD::MUL, { MVT::v8i32 }, 4 },<br>
+ { ISD::SUB, { MVT::v8i32 }, 4 },<br>
+ { ISD::ADD, { MVT::v8i32 }, 4 },<br>
+ { ISD::MUL, { MVT::v4i64 }, 4 },<br>
+ { ISD::SUB, { MVT::v4i64 }, 4 },<br>
+ { ISD::ADD, { MVT::v4i64 }, 4 },<br>
+ };<br>
+ UnaryCostTable costTable (AVX1CostTable, array_lengthof(AVX1CostTable));<br>
<br>
// Look for AVX1 lowering tricks.<br>
if (ST->hasAVX()) {<br>
- int Idx = FindInTable(AVX1CostTable, array_lengthof(AVX1CostTable), ISD,<br>
- LT.second);<br>
- if (Idx != -1)<br>
- return LT.first * AVX1CostTable[Idx].Cost;<br>
+ unsigned cost = costTable.findCost(ISD, LT.second);<br>
+ if (cost != BinaryCostTable::COST_NOT_FOUND)<br>
+ return LT.first * cost;<br>
</blockquote><div><br></div><div style>Ok, this use case makes sense: what you want is a utility to manage a table of data and do lookups within it.</div><div style><br></div><div style>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.</div>
<div style><br></div><div style>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?</div>
<div><br></div></div></div></div></div>