[Lldb-commits] [lldb] r281717 - [RenderScript] Support tracking and dumping reduction kernels
Zachary Turner via lldb-commits
lldb-commits at lists.llvm.org
Wed Sep 21 10:19:08 PDT 2016
On Wed, Sep 21, 2016 at 3:50 AM Luke Drummond <luke.drummond at codeplay.com>
wrote:
> 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).
>
A couple of advantages:
1) pointer-and-length is unsafe from a security standpoint. You run the
risk of accessing the array out of bounds, whereas ArrayRef will assert if
you try to do this, so you don't go stomp some memory and only find out
about it later.
2) ArrayRef maintains its own length and can be trivially copied, sliced
and manipulated so you can use it in a functional style. Not having to
maintain the length yourself as you iterate over it in fancy ways removes
one possible source of programmer error (plus it just makes the code look
nicer).
>
> [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.
>
Ahh, yea I see where you're doing that now. But after incrementing a line,
it's not checked whether it's at end until it gets to the next loop
iteration. This could cause a double-increment off the end right?
BTW, there's another style which you may or may not be fond of that would
also handle this case. It would go like this:
auto lines_ref = llvm::makeArrayRef(lines);
while (!lines_ref.empty()) {
auto line = lines_ref.front();
// do stuff
// ++line;
lines_ref = lines_ref.drop_front(); // instead
// end of loop
lines_ref = lines_ref.drop_front();
}
It doesn't buy you much in this case, but it can be a nice pattern
sometimes.
>
> >
> > - 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.
>
Interesting, I'll take a look.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20160921/86a79d8d/attachment-0001.html>
More information about the lldb-commits
mailing list