Page MenuHomePhabricator

dblaikie (David Blaikie)
User

Projects

User Details

User Since
Oct 8 2012, 9:19 AM (415 w, 4 d)

Recent Activity

Yesterday

dblaikie added a comment to D87272: [lld] Buffer writes when composing a single diagnostic.

Is performance important in this case? (admittedly, some folks might have builds full of warnings they aren't paying attention to & so they still care about performance then?) Generally the idea with clang/etc is that anything that produces user output (other than the successfully compiled/linked file) doesn't need to be fast (in compile/link-time terms) because it'll take /way/ longer for a user to read and think about the error message than the slight increase in execution time.

The nicer thing is that when running under strace/truss, there will be just one write for one diagnostic.

Is that true of many other tools? (I wouldn't've expected it'd be true of clang, for instance - or probably other LLVM utilities) - is it especially helpful?

The motivation was to reduce unneeded ::write calls.

Sounds like unnecessary/premature optimization, though?

I don't think it is premature optimization. When a --symbol-ordering-file has absent entries or when --call-graph-ordering-file has incorrect entries, the terminal output can be quite slow. This change can make it much faster by also making things just better (one write syscall for one line of diagnostic).

How much faster is the performance if you user disables the warning they have chosen not to fix? (if they're fixing it, the time taken to fix it will be a lot longer than the link time penalty)

https://reviews.llvm.org/D87121#2260177 I don't use it as a motivation for the patch. I created this patch because I think it is the right thing to do.

Fri, Sep 25, 12:27 PM · Restricted Project
dblaikie added inline comments to D88200: [llvm-dwarfdump][test] Rewrite verify_die_ranges.s in YAML. NFC..
Fri, Sep 25, 11:16 AM · Restricted Project

Thu, Sep 24

dblaikie updated subscribers of D87732: [Support] Provide sys::path::guess_style.

I'm not convinced this is really correct. After all, a path with mixed separators (e.g. '/my/path\to\foo') could be a Windows path rooted at the current drive. This mixed style can sometimes happen when things get concatenated together, so I don't think it's compeletely unreasonable.

I'm also concerned that if we guess wrong, the behaviour will end up breaking. For example, if the path were a Windows path "/my/path\../to/foo", I'd expect a remove_dots call (without worrying about slash normalisation) to result in "/my/to/foo", but if the code is using Posix style, will it actually result in the path being left unchanged?

I can think of two competing alternatives:

  1. add an option to remove_dots to not do the separator canonicalization. I'm not sure whether this can be done unambiguously however - which separator should be removed when a directory is removed due to dots? The one before the removed parts? After them?
  2. change all '\' to '/' unconditionally. However, this might break Linux paths with '\' in.

None of these solutions is perfect and I'm not even sure if there's one. Even if we improve remove_dots, there's still the existing issue with sys::path::append: which separator should we use when combining compilation dir, include dir and the file name.

Maybe we should instead update Clang to do the normalization when generating debug info and always use /, and then always use / unconditionally when combining paths on the consumer side (but don't do any normalization)?

You're probably right about no solution being perfect. I'm not a fan of using '/' on Windows, mostly because it looks weird, but also because at least in the past for Windows cmd, '/' paths didn't auto-complete properly. It's only a minor gripe though (and not really an issue nowadays since I mostly use PowerShell). Normalisation to at least remove dots would make a lot of sense (strings become shorter), and possibly even normalising the separator direction does too, since that would make it more likely paths end up being identical and therefore poolable in .debug_str/.debug_line_str.

sys::path::append could probably unconditionally be Posix, since such paths will also be valid Windows paths always. Maybe we need some way of communicating target platform as an alternative to native style? If that were a command-line option, the caller at least would know which style it needs to work with.

I'd like to hear others opinions - @dblaikie?

Thu, Sep 24, 6:52 PM · Restricted Project
dblaikie added a comment to D87063: [BitcodeReader] Fix O(N^2) in placeholder replacement algorithm..

OK, having a go at reviewing this (though likely you're about in the best position to assess this, Eli)...

Thu, Sep 24, 6:41 PM · Restricted Project
dblaikie accepted D88200: [llvm-dwarfdump][test] Rewrite verify_die_ranges.s in YAML. NFC..

Looks good - I don't feel that strongly about the split or merged commands.

Thu, Sep 24, 11:12 AM · Restricted Project
dblaikie accepted D88241: OpaquePtr: Add type to sret attribute.

Looks good - thanks!

Thu, Sep 24, 10:16 AM · Restricted Project
dblaikie added a comment to D81865: [clang] Use string tables for static diagnostic descriptions.

@froydnj The committed version rG31a3c5fb45b78bdaa78d94ffcc9258e839002016 appears to be very different from the review. I guess next time your probably can upload the diff again if it is very diffierent

Thu, Sep 24, 10:02 AM · Restricted Project

Wed, Sep 23

dblaikie abandoned D87937: DebugInfo: Remove DWARFv5 TUs from CUs list by sorting the finalized list.

Went with D87935 instead.

Wed, Sep 23, 10:19 PM · Restricted Project
dblaikie added a comment to D87935: DebugInfo: Filter DWARFv5 TUs out of the debug_info unit list when CUs requested.

If I had to pick between the two existing options, I think I have a marginal preference for this one, since it requires less code and is less surprising to someone who might not be too familiar with the code - I would expect a list of units to be in offset order naturally, and not expect to have to go through some sort of sorting procedure in the future.

Fair - appreciate the perspective. One other wrinkle here is that if we support the "type_units()" API (I don't think anyone's actually using it, so I'd probably delete it for now) it'll need to use a fancy iterator solution too - concat_iterator + filter_iterator to take the v5 type units out of the debug_info units, and concat that with the v4 type units from debug_types.

I'm happy for that little extra complexity. I think it intuitively makes sense, even if it does mean messing about a little in the code.

Wed, Sep 23, 10:19 PM · Restricted Project
dblaikie committed rG0328feb086fc: DebugInfo: Filter DWARFv5 TUs out of the debug_info unit list when CUs requested (authored by dblaikie).
DebugInfo: Filter DWARFv5 TUs out of the debug_info unit list when CUs requested
Wed, Sep 23, 10:16 PM
dblaikie closed D87935: DebugInfo: Filter DWARFv5 TUs out of the debug_info unit list when CUs requested.
Wed, Sep 23, 10:16 PM · Restricted Project
dblaikie added a comment to D84114: [Debuginfo] (2/8) [DW_OP_implicit_pointer/second strategy] Support for DW_OP_LLVM_explicit_pointer..

hmm, possibly more words in the patch description (& IR documentation). of this and the previous patch - what is implicit_pointer V explicit_pointer? Maybe some examples in the patch description and IR documentation about how they could be used?

Wed, Sep 23, 7:41 PM · Restricted Project, debug-info
dblaikie added inline comments to D84113: [Debuginfo] (1/8) [DW_OP_implicit_pointer/second strategy] Support for DW_OP_LLVM_implicit_pointer.
Wed, Sep 23, 7:39 PM · debug-info, Restricted Project
dblaikie accepted D87808: [DebugInfo] Fix bug in constructor homing for classes with trivial constructors..

Generally looks good to me!

Wed, Sep 23, 7:29 PM · Restricted Project
dblaikie added a comment to D68620: DebugInfo: Use base address selection entries for debug_loc.

TL;DR: It's all good.

As I worked through the calculations again myself, I realized I had forgotten that both the start and end address in a v4 entry were relocated. (The spec says neither of them are; they are supposed to be relative to the CU base address. But nobody actually does it that way.)

Wed, Sep 23, 7:07 PM · Restricted Project
dblaikie added inline comments to D87159: Add GDB prettyprinters for a few more MLIR types..
Wed, Sep 23, 6:52 PM · Restricted Project
dblaikie accepted D85670: [Instruction] Add updateLocationAfterHoist helper.

Seems good to me - minor tidy up might be possible in the unit test.

Wed, Sep 23, 6:36 PM · Restricted Project
dblaikie added a comment to D80531: [clang-tidy]: Added modernize-replace-disallow-copy-and-assign-macro.

Hi @dblaikie . I did run ninja check-all and /bin/llvm-lit -av ../clang-tools-extra/test/clang-tidy/checkers/modernize-replace-disallow-copy-and-assign-macro.cpp on this very new revision (abd70fb3983f342bc1c90f9c70a7b59790ad5206) manually but I don't see this error coming up. Can you please try to test if you see the error on a fresh build?

Wed, Sep 23, 4:14 PM · Restricted Project, Restricted Project
dblaikie added a comment to D87673: [clangd] Don't use zlib when it's unavailable..

Thanks, this seems better than crashing.
The practical result isn't wonderful: the two are going to fight over index files to some extent. But this can happen with different clangd versions (that use different index format versions) anyway. So this is really just graceful recovery from a bad situation.

Unsure about test for this

Agree, testing this seems hard and not that useful.

FWIW here's how to test codepaths only reachable when zlib is not available:

./llvm/test/tools/llvm-dwp/X86/nocompress.test:

REQUIRES: !zlib

Yep, that requires a checked-in binary file, which in the case of that test is in a standard stable format, which this data is not, so we'd also need tools to regenerate it. And we don't have APIs to generate the file without compression if zlib is available (because we never want to do that), so we'd need to add those or require anyone touching the file format to build a no-zlib tree to regenerate the file.
None of this is impossible, but this is pretty unlikely to catch a bug, and it's very unlikely to be a high-impact bug..

Wed, Sep 23, 12:45 PM · Restricted Project
dblaikie added a comment to D87673: [clangd] Don't use zlib when it's unavailable..

Thanks, this seems better than crashing.
The practical result isn't wonderful: the two are going to fight over index files to some extent. But this can happen with different clangd versions (that use different index format versions) anyway. So this is really just graceful recovery from a bad situation.

Unsure about test for this

Agree, testing this seems hard and not that useful.

Wed, Sep 23, 11:52 AM · Restricted Project

Tue, Sep 22

dblaikie added inline comments to D87159: Add GDB prettyprinters for a few more MLIR types..
Tue, Sep 22, 6:59 PM · Restricted Project
dblaikie added inline comments to D87808: [DebugInfo] Fix bug in constructor homing for classes with trivial constructors..
Tue, Sep 22, 6:55 PM · Restricted Project
dblaikie added a comment to rGb89059a31347: Revert "The wrong placement of add pass with optimizations led to -funique….

Generally it's handy to include some information about the reason for the revert in the commit message so others can follow along (makes it easier to know if a specific revert will address a breakage someone else might be seeing, etc - also useful in future review/recommit/work in this area, so there's some history of what sort of things can/did go wrong) - links to or summaries of buildbot failures, etc, are good.

Tue, Sep 22, 5:59 PM
dblaikie added a comment to D87808: [DebugInfo] Fix bug in constructor homing for classes with trivial constructors..

@rsmith What's the deal with these anonymous structs/unions? Why do they have copy/move constructors (are those technically called from the enclosing class's copy/move constructors?) but no default constructor to be called from the other ctors of the enclosing class?

Let's use the class G in the example. G::G directly initializes the field g_, so it can't call any kind of constructor for the anonymous struct member. Instead, anonymous structs and unions are "expanded" when building the constructor initializer lists for all cases other than a copy or move constructor (no initialization action is taken for union members by default, though -- not unless they have a default member initializer or an explicit mem-initializer). So the default constructor of an anonymous struct or union is never really used for anything[*].

But now consider the copy or move constructor for a type like this:

struct X {
  Y y;
  union { ... };
};

That copy or move constructor needs to build a member initializer list that (a) calls the Y copy/move constructor for y, and (b) performs a bytewise copy for the anonymous union. We can't express this in the "expanded" form, because we wouldn't know which union member to initialize.

So for the non-copy/move case, we build CXXCtorInitializers for expanded members, and in the copy/move case, we build CXXCtorInitializers calling the copy/move constructors for the anonymous structs/unions themselves.

Tue, Sep 22, 10:14 AM · Restricted Project
dblaikie added a comment to D68620: DebugInfo: Use base address selection entries for debug_loc.

Just to clarify: when I say "elf" here I'm talking about the linked executable file, and "object files" are the pre-link .o files.

Tue, Sep 22, 9:37 AM · Restricted Project
dblaikie added a comment to D87147: PR-47391 : Two DIFile entries are describing the same file two different ways.

I think the comment in https://reviews.llvm.org/rL349065 describes it quite well:

The DIFile used by the CU is special and distinct from the main source
file. Its directory part specifies what becomes the DW_AT_comp_dir
(the compilation directory), even if the source file was specified
with an absolute path.

So while the CU links to a DIFile it's more a storage mechanism for DW_AT_comp_dir and not a "file" in the normal sense and thus should be treated special. (And we might want to just store a string instead to avoid this confusion in the future.

Tue, Sep 22, 9:33 AM · debug-info

Mon, Sep 21

dblaikie accepted D87787: Temporary fix for debug loc list bug with basic block sections.

sounds good - thanks!

Mon, Sep 21, 5:26 PM · Restricted Project
dblaikie added a comment to D88048: [dwarfdump] Add verifier check to find DIEs with DW_CHILDREN_yes but without children..

maybe slightly long test case - could do this with the CU DIE itself? (perhaps a case where hand-crafted/substantially hand-modified assembly is OK, because it can be made /so/ simple - or maybe yaml2obj dwarf support is adequate for this use case?)

Mon, Sep 21, 5:20 PM · Restricted Project
dblaikie accepted D88049: [docs] Update ExtendingLLVM.rst.

Looks good - thanks!

Mon, Sep 21, 4:39 PM · Restricted Project
dblaikie added a comment to D88048: [dwarfdump] Add verifier check to find DIEs with DW_CHILDREN_yes but without children..

does this classify as a warning? Is there a distinction between ill-formed DWARF and "this is valid, but unlikely to be great"?

Mon, Sep 21, 2:48 PM · Restricted Project
dblaikie added a comment to D68620: DebugInfo: Use base address selection entries for debug_loc.

Thanks for all the data @Orlando - do you happen to have means to measure total object size too? Be useful to compare the binary size increase with the object size decrease.

Mon, Sep 21, 11:40 AM · Restricted Project
dblaikie added a comment to D87935: DebugInfo: Filter DWARFv5 TUs out of the debug_info unit list when CUs requested.

My instinct is that I prefer a third approach - simply keep both combined and separate lists - the combined list (could be a vector or an ordered map) is the offset-ordered list of all units, whilst there are then separate lists for compile and type units, with members being pointers to the units stored in the primary list. I think this has the advantage of being very explicit about things, with the drawback being that there are multiple members to keep in sync (possibly alleviated by having a wrapper class around the containers).

Mon, Sep 21, 11:39 AM · Restricted Project
dblaikie added inline comments to D87233: [POC][DebugInfo] Use entry values within IR.
Mon, Sep 21, 10:47 AM · Restricted Project, debug-info

Sat, Sep 19

dblaikie added a comment to rG0526713aa801: [FunctionAttrs] Remove redundant check. NFC.

Its two callers has excluded the possibility that
F.doesNotRecurse(), so this is safe.

Sat, Sep 19, 10:08 PM
dblaikie added a comment to rG0526713aa801: [FunctionAttrs] Remove redundant check. NFC.

Seems like this change would end up throwing off the NumNoRecurse statistic? and changes the behavior for callers (looks like it bubbles up to the "Changed" feature of passes, which means this change might cause analysis invalidation even when the IR was not changed (because the function was already marked norecurse)?

Sat, Sep 19, 9:40 PM
dblaikie accepted D87837: [Driver] Add disabled-by-default -Wuse-ld-path for the deprecation warning for -fuse-ld=/abs/path.

Good to confirm with Dave of course, but this looks pretty great to me, and thanks so much!

Sat, Sep 19, 3:40 PM · Restricted Project
dblaikie added a comment to D87837: [Driver] Add disabled-by-default -Wuse-ld-path for the deprecation warning for -fuse-ld=/abs/path.

Rather than "non-word" maybe "path" would be more legible (that's the terminology the message text uses, and seems shorter/clearer in the warning name/constant name/etc), ie: -Wuse-ld-path

Sat, Sep 19, 12:53 PM · Restricted Project

Fri, Sep 18

dblaikie committed rGad68a8b95266: DebugInfo: Cleanup RLE dumping, using a length-constrained DataExtractor rather… (authored by dblaikie).
DebugInfo: Cleanup RLE dumping, using a length-constrained DataExtractor rather…
Fri, Sep 18, 7:33 PM
dblaikie abandoned D52367: Remove address taken, add optnone.
Fri, Sep 18, 1:29 PM · Restricted Project
dblaikie commandeered D52367: Remove address taken, add optnone.
Fri, Sep 18, 1:29 PM · Restricted Project
dblaikie updated the summary of D87935: DebugInfo: Filter DWARFv5 TUs out of the debug_info unit list when CUs requested.
Fri, Sep 18, 1:23 PM · Restricted Project
dblaikie updated the summary of D87937: DebugInfo: Remove DWARFv5 TUs from CUs list by sorting the finalized list.
Fri, Sep 18, 1:23 PM · Restricted Project
dblaikie updated the summary of D87935: DebugInfo: Filter DWARFv5 TUs out of the debug_info unit list when CUs requested.
Fri, Sep 18, 1:21 PM · Restricted Project
dblaikie requested review of D87937: DebugInfo: Remove DWARFv5 TUs from CUs list by sorting the finalized list.
Fri, Sep 18, 1:19 PM · Restricted Project
dblaikie requested review of D87935: DebugInfo: Filter DWARFv5 TUs out of the debug_info unit list when CUs requested.
Fri, Sep 18, 1:11 PM · Restricted Project
Herald added a project to D52367: Remove address taken, add optnone: Restricted Project.
Fri, Sep 18, 1:09 PM · Restricted Project
dblaikie committed rG82af17cde8ca: Linewrap & remove some dead typedefs from previous commit (authored by dblaikie).
Linewrap & remove some dead typedefs from previous commit
Fri, Sep 18, 11:23 AM
dblaikie committed rG51a505340dfd: DebugInfo: Simplify line table parsing to take all the units together, rather… (authored by dblaikie).
DebugInfo: Simplify line table parsing to take all the units together, rather…
Fri, Sep 18, 11:18 AM
dblaikie committed rGe0802fe0162f: DebugInfo: Tidy up initializing multi-section contributions in DWARFContext (authored by dblaikie).
DebugInfo: Tidy up initializing multi-section contributions in DWARFContext
Fri, Sep 18, 10:55 AM

Thu, Sep 17

dblaikie updated subscribers of D87808: [DebugInfo] Fix bug in constructor homing for classes with trivial constructors..

@rsmith What's the deal with these anonymous structs/unions? Why do they have copy/move constructors (are those technically called from the enclosing class's copy/move constructors?) but no default constructor to be called from the other ctors of the enclosing class?

Thu, Sep 17, 1:26 PM · Restricted Project
dblaikie added a comment to D68620: DebugInfo: Use base address selection entries for debug_loc.

(as an aside: do you/@probinson have much of a sense of how much binary size increase you'd trade for object size reductions in this space? If binary size is ulmtimately what you care most about, I'm guessing maybe debug_loc base address specifiers will never be a win for you & perhaps we should just group them under a flag, maybe refactor the debug_ranges base address specifier flag to cover both (the flag there was introduced due to a gold+gdb_index+32 bit binary bug, unfortunately, but lumping them together seems OK-ish to me))

I'm not sure, I'll defer to @probinson on that.

What we actually care about is turnaround time, which for debug info encompasses compile time, link time, debugger startup time, and the attendant I/O latencies. Note that console download time is *not* a factor, as our downloader knows to omit the debug info.

It's not exactly raw size that matters, but we ought to care how efficiently the information is encoded in the file data. So, with respect to location lists, multiplying the number of entries without reducing the number of relocations is bad all around.

Thu, Sep 17, 1:25 PM · Restricted Project

Wed, Sep 16

dblaikie committed rG6a07f1edf8e6: debug_rnglists/symbolizing: reduce memory usage by not caching rnglists (authored by dblaikie).
debug_rnglists/symbolizing: reduce memory usage by not caching rnglists
Wed, Sep 16, 7:36 PM
dblaikie added a comment to D87787: Temporary fix for debug loc list bug with basic block sections.

Could you include the source and any command lines (clang, what optimization level, etc) was used to generate the test IR? It can make the test a bit easier to read at a glance and help if the test needs to be regenerated for some reason.

Wed, Sep 16, 7:15 PM · Restricted Project
dblaikie added a comment to D86847: [Bitcode] Add BITCODE_SIZE_BLOCK_ID to encode the size of the bitcode.

The idea is to add a terminator block, but a terminator carries less information so this patch intends to add a size block instead. With a terminator, the following problem can be better addressed

// We may be consuming bitcode from a client that leaves garbage at the end
// of the bitcode stream (e.g. Apple's ar tool). If we are close enough to
// the end that there cannot possibly be another module, stop looking.

If the container does not expect more than one bitcode stream. Then after passing one stream, it can bail out.
For users who want to have multiple streams, they can strip any padding. The scheme will automatically work with most binary formats (alignment=1).

Wed, Sep 16, 7:13 PM · Restricted Project
dblaikie added inline comments to D85293: [DebugInfo] Simplify DIEInteger::SizeOf()..
Wed, Sep 16, 7:11 PM · debug-info, Restricted Project
dblaikie added a comment to D87147: PR-47391 : Two DIFile entries are describing the same file two different ways.

Hmm, sorry for the churn here - I was going to commit this, but looking at it, it seems this patch pretty much exactly undoes the work from rL349065 - @aprantl can you weigh in on this? This patch doesn't seem to break your test - did your test test the issue correctly? Maybe some other change that's happened since your change has made reverting it viable while preserving the desired behavior?

thank you @dblaikie for commiting this.

Wed, Sep 16, 10:02 AM · debug-info
dblaikie accepted D85293: [DebugInfo] Simplify DIEInteger::SizeOf()..

Looks good - thanks!

Wed, Sep 16, 10:01 AM · debug-info, Restricted Project
dblaikie added a comment to D68620: DebugInfo: Use base address selection entries for debug_loc.

Thanks for the data!

Happy to help :)

(as an aside: do you/@probinson have much of a sense of how much binary size increase you'd trade for object size reductions in this space? If binary size is ulmtimately what you care most about, I'm guessing maybe debug_loc base address specifiers will never be a win for you & perhaps we should just group them under a flag, maybe refactor the debug_ranges base address specifier flag to cover both (the flag there was introduced due to a gold+gdb_index+32 bit binary bug, unfortunately, but lumping them together seems OK-ish to me))

I'm not sure, I'll defer to @probinson on that.

When you say "llvm-master before/after your commits" - what version of llvm-master and what commits did you have to test with? (if you can/want to discuss them)

I'm rather surprised, if master was moderately recent, that it shows no benefit from https://reviews.llvm.org/rG57d8acac64b87cb4286b00485fb2da7521fc091e (perhaps, if it's not too much hassle, you could run a sample benchmark before/after that change?)

Apologies for being unclear. Stats for "before my commits" are taken from a build at the first commit before D79571, and "with my commits" is with D86153 / rG57d8acac64b87cb4286b00485fb2da7521fc091e applied (and all the commits in between, including D82129).

Wed, Sep 16, 9:51 AM · Restricted Project

Tue, Sep 15

dblaikie added a comment to D87326: [ADT] Fix reference invalidation when self referencing a SmallVector.

I don't fundamentally mind not supporting this - especially with asan checks that might help catch clients in the codebase that violate the contract. But I'm guessing there's existing violations in the codebase given the previous patches/filed bugs about implementing this behavior.

Tue, Sep 15, 8:28 PM · Restricted Project
dblaikie added a comment to D87147: PR-47391 : Two DIFile entries are describing the same file two different ways.

Hmm, sorry for the churn here - I was going to commit this, but looking at it, it seems this patch pretty much exactly undoes the work from rL349065 - @aprantl can you weigh in on this? This patch doesn't seem to break your test - did your test test the issue correctly? Maybe some other change that's happened since your change has made reverting it viable while preserving the desired behavior?

Tue, Sep 15, 8:26 PM · debug-info
dblaikie added a comment to D87641: [DebugInfo] Add assert for variable size when creating fragments..

I'm expecting this to cause all sorts of non-clang frontends to trigger this assertion, since we don't require types to have sizes. In fact, I have an open bug for the Swift frontend to try removing the static sizes from types, since LLDB prefers the dynamic runtime size anyway.
If we want to make this a requirement, we should probably implement it as a verifier check instead of an assertion. But I'm not sure if we want to require this for all frontends.

Tue, Sep 15, 7:15 PM · Restricted Project
dblaikie added a comment to D87732: [Support] Provide sys::path::guess_style.

Hmm - probably simpler to justify this patch by including refactoring at least one (possibly more) existing uses you mentioned, to use this API - then I probably don't really need to understand what it's doing, just that it's generalizing some existing functionality.

Tue, Sep 15, 5:25 PM · Restricted Project
dblaikie added a project to D82678: [CGP] Set debug locations when optimizing phi types: debug-info.
Tue, Sep 15, 12:18 PM · debug-info, Restricted Project
dblaikie added a comment to D87326: [ADT] Fix reference invalidation when self referencing a SmallVector.

Tested the newest version of this patch and I'm still seeing massive regressions, even larger than before.

Compile-time (instructions retired): https://llvm-compile-time-tracker.com/compare.php?from=cc947207283f934c72af0eb0b1a08978c59d40a2&to=d99c6d441764431519c1c11d490e7e88ffe06775&stat=instructions This is now a 1.4% geomean regression at O3, with 2% at O3, with tramp3d-v4 hitting 3%.

Max RSS: https://llvm-compile-time-tracker.com/compare.php?from=cc947207283f934c72af0eb0b1a08978c59d40a2&to=d99c6d441764431519c1c11d490e7e88ffe06775&stat=max-rss This is now a 1.6% geomean regression at O3, with 2.1% at O0.

Clang text size goes from 80560905 to 82179908, a 2% regression (non-LTO build using GCC 9.3).

@njames93 - mind looking at the memory usage a bit? that's probably relatively easy to check (well, maybe not, since it'll be smeared over all the different instantiations of this template) & seems a bit surprising. Code growth is probably just what it is - but if there's a way to quantify it and check we've got minimal extra instantiations, that'd be good.

The compile-time stat: @nikic: can you measure a wall performance difference? (otherwise possible they're shorter instructions, etc/ doesn't necessarily mean this makes compile times worse, perhaps?)

Tue, Sep 15, 12:05 PM · Restricted Project
dblaikie added a comment to D68620: DebugInfo: Use base address selection entries for debug_loc.

@Orlando mentioned he was collecting some size data that would be relevant here, he'll post it when he's done. Basically .debug_loc sizes at various points.

Hi!

I've built a benchmark suite of 85 programs with "-O2 -g" with our downstream branch targeting X86 emitting DWARF v4.

This table provides a summary of the data. It shows the mean for some size data normalized as a percentage of the llvm-3 results for each benchmark.

For reference:
Largest binary built with llvm-3: 6010 kB
Smallest binary built with llvm-3: 79 kB

+------------------------------------------------------------------------------------------- +
| Mean binary size for benchmarks normalized as a percentage of llvm-3 builds                |
+---------------------------------+------------+------------------+-----------------+--------+
| llvm version                    | .debug_loc | other debug info | everything else | Total  |
+---------------------------------+------------+------------------+-----------------+--------+
| llvm-3                          | 13.7       | 33.2             | 53.1            | 100    |
| llvm-4                          | 12.7       | 33.8             | 53.8            | 100.3  |
| llvm-5                          | 13.4       | 35.6             | 54.6            | 103.7  |
| llvm-7                          | 18.4       | 35.6             | 54.0            | 108.0  |
| llvm-8                          | 17.5       | 37.1             | 54.5            | 109.1  |
| llvm-9                          | 19.7       | 37.2             | 54.6            | 111.5  |
| llvm-10 before dblaikie commit  | 19.8       | 37.4             | 54.9            | 112.1  |
| llvm-10 with dblaikie commit    | 25.6       | 37.4             | 54.9            | 117.9  |
| llvm-10                         | 25.8       | 37.5             | 54.8            | 118.1  |
| llvm-master before my commits   | 26.2       | 37.4             | 54.8            | 118.4  |
| llvm-master with my commits     | 18.4       | 35.5             | 55.3            | 109.3  |
+---------------------------------+------------+------------------+-----------------+--------+

Here's an image of that data in graph form: M3.

The llvm-6 entry has been omitted because the non debug-info size is a distracting outlier. The .debug_loc section size is ~16.5% for that one.

If you'd like to see the data in another format or see data for the benchmarks individually please let me know.

Tue, Sep 15, 12:03 PM · Restricted Project
dblaikie added a comment to D87326: [ADT] Fix reference invalidation when self referencing a SmallVector.

Tested the newest version of this patch and I'm still seeing massive regressions, even larger than before.

Compile-time (instructions retired): https://llvm-compile-time-tracker.com/compare.php?from=cc947207283f934c72af0eb0b1a08978c59d40a2&to=d99c6d441764431519c1c11d490e7e88ffe06775&stat=instructions This is now a 1.4% geomean regression at O3, with 2% at O3, with tramp3d-v4 hitting 3%.

Max RSS: https://llvm-compile-time-tracker.com/compare.php?from=cc947207283f934c72af0eb0b1a08978c59d40a2&to=d99c6d441764431519c1c11d490e7e88ffe06775&stat=max-rss This is now a 1.6% geomean regression at O3, with 2.1% at O0.

Clang text size goes from 80560905 to 82179908, a 2% regression (non-LTO build using GCC 9.3).

Tue, Sep 15, 1:34 AM · Restricted Project

Mon, Sep 14

dblaikie accepted D87326: [ADT] Fix reference invalidation when self referencing a SmallVector.

Great, thanks a bunch!

Mon, Sep 14, 8:27 PM · Restricted Project
dblaikie added a comment to D87326: [ADT] Fix reference invalidation when self referencing a SmallVector.

Use global namespace new inside GrowBuffer.

Why this change? Would've thought op new overloads might be desirable, but I certainly don't know the specifics off-hand right now.

SmallVector seems to use global namespace new for all its operations, so I thought I'd follow the convention there.

Mon, Sep 14, 7:49 PM · Restricted Project
dblaikie added a comment to D87657: [DebugInfo] Remove dots from getFilenameByIndex return value.

So a trivial example is:

#include <string>
int main() {}

When compiled as:

clang++ test.cc -g3 -S -o test.S

I see paths like these in the output:

"/usr/local/google/home/phosek/fuchsia" "prebuilt/third_party/clang/linux-x64/bin/../include/c++/v1/cstddef"

We could go over driver code and make sure we always invoke sys::path::remove_dots inside methods like AddIncludePaths or addPathIfExists (see for example https://github.com/llvm/llvm-project/blob/b3afad046301d8bb1f4471aceaad704b87de3a69/clang/lib/Driver/ToolChains/Gnu.cpp#L2902 which where the .. in libc++ header path comes from), but maybe it'd be better to do the normalization uniformly when emitting debuginfo metadata?

Mon, Sep 14, 7:31 PM · Restricted Project
dblaikie accepted D87656: [llvm-dwarfdump] --show-sources option to show all sources.

Sounds alright - few things could be simplified, etc.

Mon, Sep 14, 7:05 PM · Restricted Project
dblaikie accepted D87657: [DebugInfo] Remove dots from getFilenameByIndex return value.

Got any particular examples of this from LLVM's DWARF output? I suspect this sort of thing might come from/lead to path non-canonicalization which would make the line table bigger than it needs to be, so might be worth fixing there too.

Mon, Sep 14, 5:59 PM · Restricted Project
dblaikie added a comment to D87606: [ADT] Remove unnecessary copies in SmallVectorImpl::swap.

Is this testable? It should produce fewer element copies, yes? Could you add a test for it?

Mon, Sep 14, 4:44 PM · Restricted Project
dblaikie added a comment to D87326: [ADT] Fix reference invalidation when self referencing a SmallVector.

then when the call to insert etc is made the container grows a second time. In that case the element to insert would now be in freed memory which would cause asan to fail. However right now that test case doesn't seem to appear, it does happen where the container grows from small size to insert. but as the memory is stack based there, its fine, from asans POV, to reference it.
In D87237, I experimented by putting asan instrumentation inside SmallVector, which caused a test case to fail when ran under asan.

OK, so I think you're saying - the current in-tree tests, and the tests you're adding, currently don't fail even though they do test self-insertion only because they're testing in small -> big (not big -> big) mode? But with ASan instrumented SmallVector, even a small->big would cause test failures of the existing tests, without this patch to fix self-insertion?

The tests inside Constructable will cause the current in-tree self-insertion test to fail as that will detect copying from a destructed value in the call to insert. The asan instrumented SmallVector would also fail because it will detect accessing the poisoned inline buffer in the small->big growth. However with this patch both of these causes of failing tests will be fixed.

Mon, Sep 14, 4:43 PM · Restricted Project
dblaikie added a comment to D87641: [DebugInfo] Add assert for variable size when creating fragments..

I'll leave this one to @aprantl - though I am a bit concerned about whether this should be a verifier error if SROA is going to assume it's an invariant. Some discussion on the bug about whether this SROA code is only reachable for some subset of types that do have sizes, while some other types won't have sizes. (variable length arrays being a fairly fundamental example, since they can't have a known length - but I would worry that SROA might be able to mess with known portions of them, maybe?) I mean, I'm usually all for "throw an assert in, and if it ever fails then we'll have figured out what case we didn't consider earlier", though just a bit hesitant on this at the moment. Probably no big deal, though.

Mon, Sep 14, 4:29 PM · Restricted Project
dblaikie added a comment to D87560: Clarify two data member names.

The patch has been pushed.

Mon, Sep 14, 3:29 PM · Restricted Project
dblaikie added a comment to D68620: DebugInfo: Use base address selection entries for debug_loc.

@probinson - do you have any state here? Whether this is an ongoing issue, whether the improvements to avoid unnecessary debug_loc have reduced the overhead sufficiently, etc?

@Orlando mentioned he was collecting some size data that would be relevant here, he'll post it when he's done. Basically .debug_loc sizes at various points.

Mon, Sep 14, 9:53 AM · Restricted Project
dblaikie accepted D87147: PR-47391 : Two DIFile entries are describing the same file two different ways.

Couple of things to clean up in the test, but otherwise looks good. Appreciate your patience with all the fussy details.

Mon, Sep 14, 9:23 AM · debug-info

Sun, Sep 13

dblaikie added a comment to D87147: PR-47391 : Two DIFile entries are describing the same file two different ways.

Can the test be reduced down to "extern int i; int i;" now?

Sun, Sep 13, 1:43 PM · debug-info
dblaikie added a comment to D87326: [ADT] Fix reference invalidation when self referencing a SmallVector.

Yeah, seems that SmallVector::push_back should be fixed to allow a.push_back(a[0]). SmallVector::insert has been fixed in rL134554

It hasn't, that handles the case where the element to insert is moved when shifting the elements down to make room, but not when the element to insert is moved because of container growth.

Sun, Sep 13, 1:42 PM · Restricted Project
dblaikie updated subscribers of D87237: [ADT] Add ASAN Support for SmallVector.
Sun, Sep 13, 1:35 PM · Restricted Project
dblaikie committed rGce89eeee16dd: PPCInstrInfo: Fix readability-inconsistent-declaration-parameter-name clang… (authored by dblaikie).
PPCInstrInfo: Fix readability-inconsistent-declaration-parameter-name clang…
Sun, Sep 13, 1:11 PM
dblaikie committed rG7940af02baa2: Correct end-of-namespace comment to be clang-tidy/LLVM style appropriate (authored by dblaikie).
Correct end-of-namespace comment to be clang-tidy/LLVM style appropriate
Sun, Sep 13, 1:11 PM
dblaikie committed rG6e06f1cd0816: GCOVProfiling: Avoid use-after-move (authored by dblaikie).
GCOVProfiling: Avoid use-after-move
Sun, Sep 13, 1:11 PM

Sat, Sep 12

dblaikie added a comment to D87326: [ADT] Fix reference invalidation when self referencing a SmallVector.

Yeah, seems that SmallVector::push_back should be fixed to allow a.push_back(a[0]). SmallVector::insert has been fixed in rL134554

Ah, right, sort of a related but maybe slightly different problem - when not resizing the underlying buffer, elements are moved, then the new one is inserted so the reference may've been invalidated. Probably best to handle that in a separate/follow-up commit - any idea if the C++ standard guarantees this for std::vector, for instance? And if so, how does libc++ implement this guarantee?

Sat, Sep 12, 5:07 PM · Restricted Project
dblaikie added a comment to D87326: [ADT] Fix reference invalidation when self referencing a SmallVector.

Hmm - I think maybe what I had trouble with was understanding how much this shuold generalize. Should this handle subranges? (if you try to append half of a vector onto itself) and if so, how? Anyone happen to know what's guaranteed by the standard (perhaps only push_back of single elements - that's easier to handle)

From what I can see insert and append fully support when the range to insert is enclosed by the vector, however assign makes no such promise as it would potentially require allocating more storage even if the container itself doesn't need to grow

Sat, Sep 12, 5:06 PM · Restricted Project
dblaikie added a comment to D87147: PR-47391 : Two DIFile entries are describing the same file two different ways.

extern int i;
int i;

We are not getting 2 DIFile entries for the above case,for your reference .

!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
!1 = distinct !DIGlobalVariable(name: "i", scope: !2, file: !3, line: 2, type: !6, isLocal: false, isDefinition: true)
!2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang version 10.0.0.1 (http://ukallapa@stash.wrs.com/scm/llvm/clang.git 2660105769d58dbb8b66f56247f208ca2872913a) (llvm/llvm.git bfc5adc611fdc055ece9191c3d8df0750bec8816)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, globals: !5, splitDebugInlining: false, nameTableKind: None)
!3 = !DIFile(filename: "test.c", directory: "/folk/ukallapa")
!4 = !{}
!5 = !{!0}
!6 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!7 = !{i32 7, !"Dwarf Version", i32 4}
!8 = !{i32 2, !"Debug Info Version", i32 3}
!9 = !{i32 1, !"wchar_size", i32 4}
!10 = !{!"clang version 10.0.0.1 (http://

Sat, Sep 12, 4:46 PM · debug-info

Fri, Sep 11

dblaikie added a comment to D87326: [ADT] Fix reference invalidation when self referencing a SmallVector.

I believe I filed the second bug ( https://bugs.llvm.org/show_bug.cgi?id=16253 ) a while back because I tried to do it myself (admittedly, it was myself 7 years ago, so maybe my perspective has changed over the years) and found it too difficult. That there were corner cases or something that made me unsure how to move forward - so I filed the bug and forgot about it.

Fri, Sep 11, 7:12 PM · Restricted Project
dblaikie added a comment to D86926: FormatTest: Provide real line number in failure messages.

Could this be done with a custom matcher, perhaps? (might be a bit tidier than macros with FILE and LINE manually handled, etc?

Fri, Sep 11, 6:45 PM · Restricted Project
dblaikie accepted D87553: [gn] Remove unneeded MC dep from llvm-tblgen.

Sounds good to me

Fri, Sep 11, 6:27 PM · Restricted Project
dblaikie committed rG928d419797ea: Fix a couple of tests that relied on the clang binary having 'clang' somewhere… (authored by dblaikie).
Fix a couple of tests that relied on the clang binary having 'clang' somewhere…
Fri, Sep 11, 6:10 PM
dblaikie added inline comments to D87147: PR-47391 : Two DIFile entries are describing the same file two different ways.
Fri, Sep 11, 4:25 PM · debug-info
dblaikie accepted D87026: [DebugInfo] Make offsets of dwarf units 64-bit (19/19)..
Fri, Sep 11, 4:22 PM · debug-info, Restricted Project
dblaikie added inline comments to D87008: [DebugInfo] Fix methods of AsmPrinter to emit values corresponding to the DWARF format (1/19)..
Fri, Sep 11, 4:20 PM · debug-info, Restricted Project
dblaikie added a comment to D87009: [DebugInfo] Fix DIE value emitters to be compatible with DWARF64 (2/19)..

Let's take, for example, DIEExpr. As far as I can find, with DW_FORM_sec_offset it is used only in the DebugInfo/DWARF unit tests, in dwarfgen::DIE::addStrOffsetsBaseAttribute(). If we took the approach with end-to-end functional testing, this class would not be updated. And the path is not simple to track, by the way.

API only intended for unit tests would be tested/updated with unit tests.

The DIEExpr class is not intended to be used only in unit tests; the CodeGen library uses it, but with different DWARF forms. The generator in DebugInfo/DWARF unit tests does not test the class, it just uses it to create some bits of DWARF data. Moreover, these unit tests are for a different library and as such are not the best place to be updated if we want to fix the issue in a class of the CodeGen library. It is much better to test the implementation in the library's own unit tests, so this patch does exactly that. I do not want to leave DIEExpr in the not updated state, because the new code may use it without noticing that the implementation is broken for DWARF64.

Fri, Sep 11, 9:21 AM · debug-info, Restricted Project
dblaikie added a comment to D87021: [DebugInfo] Fix emitting DWARF64 type units (10/19)..

The test sources were based on some simple code, but then they were cleared as much as I could.

You mean the IR was modified after compilation? Or that the original C source was simplified as much as possible?

I removed/adjusted some attributes here and there to make the tests smaller and to express mostly what I wanted to test in each particular case. In some cases, like the test for .debug_macro, the source was created fully manually, based on existing tests.

Usually we don't simplify the IR - not worth it/doesn't make things especially more maintainable & more likely to introduce something that makes the IR quirky/not representative of the normal Clang generated IR.

I'll see if I can update the tests in the series so that the IR is fully generated by clang.

Fri, Sep 11, 9:12 AM · debug-info, Restricted Project

Thu, Sep 10

dblaikie accepted D87489: [NFC][MLInliner] Presort instruction successions..

Looks good - thanks!

Thu, Sep 10, 8:54 PM · Restricted Project
dblaikie accepted D87010: [DebugInfo] Add new emitting methods for values which depend on the DWARF format (3/19)..

Looks good - thanks!

Thu, Sep 10, 7:11 PM · debug-info, Restricted Project
dblaikie accepted D87023: [DebugInfo] Fix emitting DWARF64 .debug_names sections (16/19)..

Looks good, thanks!

Thu, Sep 10, 7:09 PM · debug-info, Restricted Project
dblaikie accepted D87008: [DebugInfo] Fix methods of AsmPrinter to emit values corresponding to the DWARF format (1/19)..
Thu, Sep 10, 7:08 PM · debug-info, Restricted Project
dblaikie added a comment to D87159: Add GDB prettyprinters for a few more MLIR types..

Hi David, is it OK if I send these changes your way for review? There is maybe one or two more after that for now.

This revision primarily adds some helpers to skip printing wrapper types through inheritance or composition.

Thanks!

Thu, Sep 10, 7:00 PM · Restricted Project