[PATCH] D22792: VecClone Pass

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: include/llvm/Analysis/VectorVariant.h:38
+
+class VectorKind {
+
----------------
Description of the class?


================
Comment at: include/llvm/Analysis/VectorVariant.h:41
+public:
+  enum ParmKind {
+    StrideParmKind = 's',
----------------
Explicitly set the base type to `char`


================
Comment at: include/llvm/Analysis/VectorVariant.h:48
+
+  VectorKind(char K, int S, int A = NOT_ALIGNED) {
+
----------------
Use member initializers


================
Comment at: include/llvm/Analysis/VectorVariant.h:69
+
+  VectorKind(const VectorKind &Other) {
+    Kind = Other.Kind;
----------------
`VectorKind(const VectorKind &Other) = default;`


================
Comment at: include/llvm/Analysis/VectorVariant.h:75
+
+  /// \brief Is the stride for a linear parameter a uniform variable? (i.e.,
+  /// the stride is stored in a variable but is uniform)
----------------
No need to use `\brief` tag, you must remove it


================
Comment at: include/llvm/Analysis/VectorVariant.h:77
+  /// the stride is stored in a variable but is uniform)
+  bool isVariableStride() { return Kind == StrideParmKind; }
+
----------------
const function


================
Comment at: include/llvm/Analysis/VectorVariant.h:80
+  /// \brief Is the stride for a linear variable non-unit stride?
+  bool isNonUnitStride() { return Kind == LinearParmKind && Stride != 1; }
+
----------------
const function


================
Comment at: include/llvm/Analysis/VectorVariant.h:83
+  /// \brief Is the stride for a linear variable unit stride?
+  bool isUnitStride() { return Kind == LinearParmKind && Stride == 1; }
+
----------------
const function


================
Comment at: include/llvm/Analysis/VectorVariant.h:86
+  /// \brief Is this a linear parameter?
+  bool isLinear() {
+    return isVariableStride() || isNonUnitStride() || isUnitStride();
----------------
const function


================
Comment at: include/llvm/Analysis/VectorVariant.h:91
+  /// \brief Is this a uniform parameter?
+  bool isUniform() { return Kind == UniformParmKind; }
+
----------------
const function


================
Comment at: include/llvm/Analysis/VectorVariant.h:94
+  /// \brief Is this a vector parameter?
+  bool isVector() { return Kind == VectorParmKind; }
+
----------------
const function


================
Comment at: include/llvm/Analysis/VectorVariant.h:97
+  /// \brief Is the parameter aligned?
+  bool isAligned() { return Alignment != NOT_ALIGNED; }
+
----------------
const function


================
Comment at: include/llvm/Analysis/VectorVariant.h:100
+  /// \brief Get the stride associated with a linear parameter.
+  int getStride() { return Stride; }
+
----------------
const function


================
Comment at: include/llvm/Analysis/VectorVariant.h:103
+  /// \brief Get the alignment associated with a linear parameter.
+  int getAlignment() { return Alignment; }
+
----------------
const function


================
Comment at: include/llvm/Analysis/VectorVariant.h:111
+  /// corresponding to the standards defined in the vector function ABI.
+  std::string encode() {
+    std::stringstream SST;
----------------
Why not `SmallString`?


================
Comment at: include/llvm/Analysis/VectorVariant.h:112
+  std::string encode() {
+    std::stringstream SST;
+    SST << Kind;
----------------
Why not `llvm::raw_svector_ostream`?


================
Comment at: include/llvm/Analysis/VectorVariant.h:132
+private:
+  char Kind;      // linear, uniform, vector
+  int  Stride;
----------------
`char`->`ParmKind`


================
Comment at: include/llvm/Analysis/VectorVariant.h:144
+  unsigned int Vlen;
+  std::vector<VectorKind> Parameters;
+
----------------
Use `SmallVector` or `ArrayRef`


================
Comment at: include/llvm/Analysis/VectorVariant.h:146
+
+  static std::string prefix() { return "_ZGV"; }
+
----------------
`std::string`->`StringRef`. Is this target-specific?


================
Comment at: include/llvm/Analysis/VectorVariant.h:152
+  /// \brief Get the ISA corresponding to this vector variant.
+  int getISA() { return ISAClass; }
+
----------------
const function


================
Comment at: include/llvm/Analysis/VectorVariant.h:155
+  /// \brief Is this a masked vector function variant?
+  bool isMasked() { return Mask; }
+
----------------
const function


================
Comment at: include/llvm/Analysis/VectorVariant.h:158
+  /// \brief Get the vector length of the vector variant.
+  unsigned int getVlen() { return Vlen; }
+
----------------
const function


================
Comment at: include/llvm/Analysis/VectorVariant.h:161
+  /// \brief Get the parameters of the vector variant.
+  std::vector<VectorKind> &getParameters() { return Parameters; }
+
----------------
const function


================
Comment at: include/llvm/Analysis/VectorVariant.h:166
+  /// mask information, and all parameters.
+  std::string encode() {
+
----------------
`SmallString`


================
Comment at: include/llvm/Analysis/VectorVariant.h:166
+  /// mask information, and all parameters.
+  std::string encode() {
+
----------------
ABataev wrote:
> `SmallString`
const function


================
Comment at: include/llvm/Analysis/VectorVariant.h:186
+  /// \brief Generate a function name corresponding to a vector variant.
+  std::string generateFunctionName(StringRef ScalarFuncName) {
+    std::string Name = encode();
----------------
const function


================
Comment at: include/llvm/Analysis/VectorVariant.h:193
+  /// that is supported.
+  Type *promoteToSupportedType(Type *Ty) {
+    return TTI->promoteToSupportedType(Ty, getISA());
----------------
const function


================
Comment at: include/llvm/Analysis/VectorVariant.h:206
+
+    return EncodeMask ? 'M' : 'N';
+  }
----------------
Target-specific?


================
Comment at: include/llvm/Analysis/VectorVariant.h:212
+
+    switch (MaskToDecode) {
+    case 'M':
----------------
Target-specific?


================
Comment at: include/llvm/Transforms/Utils/VecClone.h:31
+/// \brief Contains the names of the declared vector function variants
+typedef std::vector<StringRef> DeclaredVariants;
+
----------------
SmallVector


================
Comment at: include/llvm/Transforms/Utils/VecClone.h:34
+/// \brief Contains a mapping of a function to its vector function variants
+typedef std::map<Function*, DeclaredVariants> FunctionVariants;
+
----------------
DenseMap


================
Comment at: include/llvm/Transforms/Utils/VecClone.h:41
+  /// list of variants.
+  void getFunctionsToVectorize(Module &M, FunctionVariants &FuncVars);
+
----------------
const function


================
Comment at: include/llvm/Transforms/Utils/VecClone.h:51
+  template <typename T>
+  Constant* getConstantValue(Type *Ty, LLVMContext &Context, T Val);
+
----------------
const function


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D22792/new/

https://reviews.llvm.org/D22792