Hi Zachary

Thanks for the review. My comments are inline (I've cut down some of the diff context for brevity)


On 16/09/16 15:57, Zachary Turner wrote:
[snip]
    -#define MAXLINESTR_(x) "%" STRINGIFY(x) "s"
    -#define MAXLINESTR MAXLINESTR_(MAXLINE)
    +bool RSModuleDescriptor::ParsePragmaCount(llvm::StringRef *lines,
    +                                          size_t n_lines) {
    +  // Skip the pragma prototype line
    +  ++lines;
    +  for (; n_lines--; ++lines) {
    +    const auto kv_pair = lines->split(" - ");
    +    m_pragmas[kv_pair.first.trim().str()] =
    kv_pair.second.trim().str();
    +  }
    +  return true;
    +}
    +
    +bool RSModuleDescriptor::ParseExportReduceCount(llvm::StringRef *lines,
    +                                                size_t n_lines) {

This should take an ArrayRef<StringRef>.  In general this is true any
time you're passing an array to a function via (T* items, size_t length).


    +  // The list of reduction kernels in the `.rs.info
    <http://rs.info>` symbol is of the form
    +  // "signature - accumulatordatasize - reduction_name -
    initializer_name -
    +  // accumulator_name - combiner_name -
    +  // outconverter_name - halter_name"
    +  // Where a function is not explicitly named by the user, or is
    not generated
    +  // by the compiler, it is named "." so the
    +  // dash separated list should always be 8 items long
    +  Log *log = GetLogIfAllCategoriesSet(LIBLLDB_LOG_LANGUAGE);
    +  // Skip the exportReduceCount line
    +  ++lines;
    +  for (; n_lines--; ++lines) {

With ArrayRef this would be written as:

for (StringRef line : lines.drop_front()) {
}

I'm not sure what the technical argument against pointer and length vs ArrayRef is, but I agree these range-based for loops are pretty. I'll add them in a future commit (I've got some more RenderScript cleanups pending, and I'll test this locally first - but this looks like good cleanup).

[snip]
    __attribute__((kernel))
    +       {"exportForEachCount", eExportForEach},
    +       // The number of generalreductions: This marked in the
    script by `#pragma
    +       // reduce()`
    +       {"exportReduceCount", eExportReduce},
    +       // Total count of all RenderScript specific `#pragmas` used
    in the script
    +       {"pragmaCount", ePragma},
    +       {"objectSlotCount", eObjectSlot}}};

       // parse all text lines of .rs.info <http://rs.info>
       for (auto line = info_lines.begin(); line != info_lines.end();
    ++line) {

How about (for auto line : info_lines) {

We actually need to manually increment the line pointer to skip over the already-parsed lines, so the range based for loop doesn't work in this case.


    -    uint32_t numDefns = 0;
    -    if (sscanf(line->c_str(), "exportVarCount: %" PRIu32 "",
    &numDefns) == 1) {
    -      while (numDefns--)
    -        m_globals.push_back(RSGlobalDescriptor(this,
    (++line)->c_str()));
    -    } else if (sscanf(line->c_str(), "exportForEachCount: %" PRIu32 "",
    -                      &numDefns) == 1) {
    -      while (numDefns--) {
    -        uint32_t slot = 0;
    -        name[0] = '\0';
    -        static const char *fmt_s = "%" PRIu32 " - " MAXLINESTR;
    -        if (sscanf((++line)->c_str(), fmt_s, &slot, name.data()) ==
    2) {
    -          if (name[0] != '\0')
    -            m_kernels.push_back(RSKernelDescriptor(this,
    name.data(), slot));
    -        }
    -      }
    -    } else if (sscanf(line->c_str(), "pragmaCount: %" PRIu32 "",
    &numDefns) ==
    -               1) {
    -      while (numDefns--) {
    -        name[0] = value[0] = '\0';
    -        static const char *fmt_s = MAXLINESTR " - " MAXLINESTR;
    -        if (sscanf((++line)->c_str(), fmt_s, name.data(),
    value.data()) != 0) {
    -          if (name[0] != '\0')
    -            m_pragmas[std::string(name.data())] = value.data();
    -        }
    -      }
    -    } else {
    -      Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_LANGUAGE));
    -      if (log) {
    +    const auto kv_pair = line->split(": ");
    +    const auto key = kv_pair.first;
    +    const auto val = kv_pair.second.trim();
    +
    +    const auto handler = rs_info_handlers.find(key);
    +    if (handler == rs_info_handlers.end())
    +      continue;

Instead of constructing this map, you could use llvm::StringSwitch, like
this:
int handler = llvm::StringSwitch<int>(key)
                        .Case("exportVarCount", eExportVar)
                        .Case("exportForEachCount", eExportForEach)
                        .Case("exportReduceCount", eExportReduce)
                        .Case("pragmaCount", ePragma)
                        .Case("objectSlotCount", eObjectSlot)
                        .Default(-1);


I wasn't aware at all of `llvm::StringSwitch`. This seems like it is more fit for purpose than what I had. Will add.

[snip]
    @@ -110,6 +162,7 @@ public:
       const lldb::ModuleSP m_module;
       std::vector<RSKernelDescriptor> m_kernels;
       std::vector<RSGlobalDescriptor> m_globals;
    +  std::vector<RSReductionDescriptor> m_reductions;
       std::map<std::string, std::string> m_pragmas;

You should consider changing this to llvm::StringMap, which has better
performance characteristics than std::map

For the case of a small number of keys (which is certainly true for the `m_pragmas` case), I've found `std::map` to have significantly better performance characteristics in artificial tests when compared to `llvm::StringMap` (x86_64 linux gcc-6.2 with libstdc++) so I'm, not too keen to replace all instances of `std::map<string, T>` at this time. In many cases though, I think the llvm containers (particularly StringRef) have a number of advantages over the standard library, and I'll definitely start moving over where it makes sense.

I threw together a very simple testcase for these performance numbers, which is attached.


Best

Luke

#include <chrono>
#include <map>
#include <iostream>

#include <llvm/ADT/StringRef.h>
#include <llvm/ADT/StringSwitch.h>
#include <llvm/ADT/StringMap.h>
#include <llvm/ADT/ArrayRef.h>

using namespace llvm;

enum { eVal0, eVal1, eVal2, eVal3, eVal4, eVal5, eVal6, eVal7, eVal8 };

#define ITERATIONS (100000000u)


int stdmap_switch(const StringRef key) {
  static const std::map<StringRef, int> lut{
      {"value1", eVal1},
      {"value2", eVal2},
      {"value3", eVal3},
      {"value4", eVal4},
      {"value5", eVal5},
      {"value6", eVal6},
      {"value7", eVal7},
      {"value8", eVal8},
  };
  const auto val = lut.find(key);
  if (val == lut.end())
    return -1;
  return val->second;
}

int stringmap_switch(const StringRef key) {
  static const StringMap<int> lut{
      {"value1", eVal1},
      {"value2", eVal2},
      {"value3", eVal3},
      {"value4", eVal4},
      {"value5", eVal5},
      {"value6", eVal6},
      {"value7", eVal7},
      {"value8", eVal8},
  };
  const auto &val = lut.find(key);
  if (val == lut.end())
    return -1;
  return val->getValue();
}

int llvm_stringswitch(const StringRef key) {
  return StringSwitch<int>(key)
      .Case("value1", eVal1)
      .Case("value2", eVal2)
      .Case("value3", eVal3)
      .Case("value4", eVal4)
      .Case("value5", eVal5)
      .Case("value6", eVal6)
      .Case("value7", eVal7)
      .Case("value8", eVal8)
      .Default(-1);
}

int main(int argc, char **argv) {
  ArrayRef<char *> arr{argv, (size_t)argc};

  std::chrono::steady_clock timer;
  int val = 0;
  auto start_stringmap_switch = timer.now();
  for (const auto &arg : arr.drop_front()) {
    for (size_t i = 0; i < ITERATIONS; ++i)
      val |= stringmap_switch(arg);
  }
  auto stop_stringmap_switch = timer.now();

  auto start_stdmap_switch = timer.now();
  for (const auto &arg : arr.drop_front()) {
    for (size_t i = 0; i < ITERATIONS; ++i)
      val |= stdmap_switch(arg);
  }
  auto stop_stdmap_switch = timer.now();

  auto start_llvm_stringswitch = timer.now();
  for (const auto &arg : arr.drop_front()) {
    for (size_t i = 0; i < ITERATIONS; ++i)
      val |= llvm_stringswitch(arg);
  }
  auto stop_llvm_stringswitch = timer.now();

  for (const auto &arg : arr.drop_front()) {
    ::printf("%s -> %d\n", arg, stdmap_switch(arg));
    ::printf("%s -> %d\n", arg, stringmap_switch(arg));
    ::printf("%s -> %d\n", arg, llvm_stringswitch(arg));
  }

  ::printf("std::map took %g seconds\n",
           std::chrono::duration<double>(stop_stdmap_switch -
                                              start_stdmap_switch)
               .count());

  ::printf("StringMap::find took %g seconds\n",
           std::chrono::duration<double>(stop_stringmap_switch -
                                         start_stringmap_switch)
               .count());

  ::printf("llvm::StringSwitch took %g seconds\n",
           std::chrono::duration<double>(stop_llvm_stringswitch -
                                         start_llvm_stringswitch)
               .count());

  return val;
}

import os

env = Environment(ENV=os.environ)
env['CXX'] = os.environ.get('CXX', 'c++')
env.ParseConfig('llvm-config --cxxflags --ldflags --libs')
env.Append(LIBS=['dl', 'pthread', 'dl', 'z'])
env.Append(CXXFLAGS=['-Wall', '-Wextra', '-Ofast', '-mtune=native'])

env.Program('mapvswitch', 'mapvswitch.cpp')
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to