snehasish accepted this revision.
snehasish added a comment.
This revision is now accepted and ready to land.
lgtm
================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1276
+ if (Error E = MemProfResult.takeError()) {
+ handleAllErrors(std::move(E), [&](const InstrProfError &IPE) {
+ auto Err = IPE.get();
----------------
tejohnson wrote:
> snehasish wrote:
> > Consider defining the lambda outside above the condition to reduce
> > indentation. IMO it will be a little easier to follow if it wasn't inlined
> > into the if statement itself.
> I could do this, but as is it mirrors the structure of the similar handling
> in readCounters, which has some advantages. wdyt?
I wasn't a big fan of the existing structure in readCounters but I didn't want
to ask you to change the other code. Let's leave it as is for now.
================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1360
+ // If leaf was found in a map, iterators pointing to its location in both
+ // of the maps (it may only exist in one).
+ std::map<uint64_t, std::set<const AllocationInfo *>>::iterator
----------------
tejohnson wrote:
> snehasish wrote:
> > Can you add an assert for this?
> In this case "may only" meant "might only", not "may only at most". So I
> can't assert on anything. This can happen for example if we have a location
> that corresponds to both an allocation call and another callsite (I've seen
> this periodically, and can reproduce e.g. with a macro). We would need to use
> discriminators more widely to better distinguish them in that case (with the
> handling here we will only match to the allocation call for now - edit: a
> slight change noted further below ensures this is the case). Will change
> /may/might/ and add a note.
Thanks for the explanation.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128142/new/
https://reviews.llvm.org/D128142
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
