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

[Chandler Carruth]

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.

Some detailed comments below to try to help you formulate a new version....

On Wed, Jan 16, 2013 at 1:29 PM, Renato Golin <renato.golin at linaro.org>wrote:

> Author: rengolin
> Date: Wed Jan 16 15:29:55 2013
> New Revision: 172658
>
> URL: http://llvm.org/viewvc/llvm-project?rev=172658&view=rev
> Log:
> Change CostTable model to be global to all targets
>
> Moving the X86CostTable to a common place, so that other back-ends
> can share the code. Also simplifying it a bit and commoning up
> tables with one and two types on operations.
>
> Modified:
>     llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h
>     llvm/trunk/lib/Analysis/TargetTransformInfo.cpp
>     llvm/trunk/lib/Target/X86/X86TargetTransformInfo.cpp
>
> Modified: llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h?rev=172658&r1=172657&r2=172658&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h (original)
> +++ llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h Wed Jan 16
> 15:29:55 2013
> @@ -27,6 +27,7 @@
>  #include "llvm/IR/Type.h"
>  #include "llvm/Pass.h"
>  #include "llvm/Support/DataTypes.h"
> +#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.


>
> +//======================================= COST TABLES ==
>

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


> +
> +/// \brief An entry in a cost table
> +///
> +/// Use it as a static array and call the CostTable below to
> +/// iterate through it and find the elements you're looking for.
> +///
> +/// Leaving Types with fixed size to avoid complications during
> +/// static destruction.
> +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.


> +
> +/// \brief Cost table, containing one or more costs for different
> instructions
> +///
> +/// This class implement the cost table lookup, to simplify
> +/// how targets declare their own costs.
> +class CostTable {
> +  const CostTableEntry *table;
> +  const size_t size;
> +  const unsigned numTypes;
>

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.

+
> +protected:
> +  /// Searches for costs on the table
> +  unsigned _findCost(int ISD, MVT *Types) const;
>
+
> +  // We don't want to expose a multi-type cost table, since types are not
> +  // sequential by nature. If you need more cost table types, implement
> +  // them below.
> +  CostTable(const CostTableEntry *table, const size_t size, unsigned
> numTypes);
> +
> +public:
> +  /// Cost Not found while searching
> +  static const unsigned COST_NOT_FOUND = -1;
> +};
>

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


> +
> +/// Specialisation for one-type cost table
> +class UnaryCostTable : public CostTable {
> +public:
> +  UnaryCostTable(const CostTableEntry *table, const size_t size);
> +  unsigned findCost(int ISD, MVT Type) const;
> +};
>

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.


OK, let's look at how you're using it:


> -  static const X86CostTblEntry AVX1CostTable[] = {
> -    // We don't have to scalarize unsupported ops. We can issue two
> half-sized
> -    // operations and we only need to extract the upper YMM half.
> -    // Two ops + 1 extract + 1 insert = 4.
> -    { ISD::MUL,     MVT::v8i32,    4 },
> -    { ISD::SUB,     MVT::v8i32,    4 },
> -    { ISD::ADD,     MVT::v8i32,    4 },
> -    { ISD::MUL,     MVT::v4i64,    4 },
> -    { ISD::SUB,     MVT::v4i64,    4 },
> -    { ISD::ADD,     MVT::v4i64,    4 },
> -    };
> +  // We don't have to scalarize unsupported ops. We can issue two
> half-sized
> +  // operations and we only need to extract the upper YMM half.
> +  // Two ops + 1 extract + 1 insert = 4.
> +  static const CostTableEntry AVX1CostTable[] = {
> +    { ISD::MUL,    { MVT::v8i32 },    4 },
> +    { ISD::SUB,    { MVT::v8i32 },    4 },
> +    { ISD::ADD,    { MVT::v8i32 },    4 },
> +    { ISD::MUL,    { MVT::v4i64 },    4 },
> +    { ISD::SUB,    { MVT::v4i64 },    4 },
> +    { ISD::ADD,    { MVT::v4i64 },    4 },
> +  };
> +  UnaryCostTable costTable (AVX1CostTable, array_lengthof(AVX1CostTable));
>
>    // Look for AVX1 lowering tricks.
>    if (ST->hasAVX()) {
> -    int Idx = FindInTable(AVX1CostTable, array_lengthof(AVX1CostTable),
> ISD,
> -                          LT.second);
> -    if (Idx != -1)
> -      return LT.first * AVX1CostTable[Idx].Cost;
> +    unsigned cost = costTable.findCost(ISD, LT.second);
> +    if (cost != BinaryCostTable::COST_NOT_FOUND)
> +      return LT.first * cost;
>

Ok, this use case makes sense: what you want is a utility to manage a table
of data and do lookups within it.

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.

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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130120/8b1e8c20/attachment.html>