User Details
- User Since
- Mar 28 2019, 8:58 AM (95 w, 3 d)
Tue, Jan 19
Thanks @rnk
Fri, Jan 15
Thu, Jan 14
Address feedback from @bjope:
+ Change comment for dbg.value+derefs in ShouldRemove lambda.
+ Update the test to check that the backward scan removes adjacent duplicate dbg.value+derefs still.
Rebase
Rebase and fix a typo in a comment.
Nov 24 2020
Nov 23 2020
Nov 18 2020
Thank you @bjope for taking a look at this.
Nov 13 2020
Nov 9 2020
Jeremy said:
In theory this kind of behaviour is exactly what dbg.addr is for (variable lives in memory, maybe changes due to control flow). However moving everything to use dbg.addr stalled a long time ago, I believe @Orlando found that it's often unexpectedly dropped by optimisations, any opinion on whether it's usable @Orlando? We're hoping to use it for things someday soon, but we're definitely not there yet.
Oct 26 2020
Thanks. I tried running these on Windows but ran into issues with the dexter args so I landed these with UNSUPPORTED: system-windows.
Oct 23 2020
Addressed Reid's comments:
+ Converted all tests to use attributes to control inlining behaviour (as opposed to a mix of this and clang flags).
+ Improved comments in some tests.
Apologies, in shortening the commit message I carelessly removed the review number. This was committed in fea067bdfde4.
Oct 21 2020
Addressed @vsk's comments:
+ Resize SmallVector<...,8> -> SmallVector<...,1>
+ Rename Declares -> DbgUsers.
Hi @vsk, thanks for taking a look, I'll address your inline comments shortly.
Oct 20 2020
FWIW, ignoring the test style discussion, the change LGTM.
AFAICT, prior to D80264 these dbg.value+derefs would've been silently dropped instead of made undef, so we're really just returning to previous behaviour with this patch. I've got a patch brewing which applies this fix to mem2reg which I'll upload shortly.
Oct 19 2020
Thanks for taking a look @rnk. Your comment on struct-dse.c reminds me that I left out some important info in the description. All of these except ctrl-flow.c "fail" (score < 1.0) at the moment. Should they be marked XFAIL?
Oct 16 2020
Sep 23 2020
Sep 22 2020
I guess all of these measurements were done without Split DWARF (shouldn't make things better/worse overall (across .o and .dwo), really, compared to DWARFv5 non-split - but means when only looking at .o files the difference of avoiding more debug_addr entries is more significant because there's fewer remaining .o debug bytes to begin with) and without compression (-gz) enabled?
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.
The following builds are with clang @ 485e6db8729 (3rd September) targeting x86.
+---------------------------------------------------------------+ | File size (bytes) of clang-3.4 built with -O2 -gdwarf-4 | *===============================================================* | | base addr | no base addr | % change | +--------------------+--------------+--------------+------------+ | Accumulated object | 1874653208 | 1924003152 | +2.63 | | file sizes | | | | +--------------------+--------------+--------------+------------+ | Elf size | 527591064 | 513946184 | -2.57 | +--------------------+--------------+--------------+------------+
Sep 21 2020
Out of curiosity I also did a clang-3.4 build too using master @ 485e6db8729 (3rd September) with "-O2 -gdwarf-4". It is smaller when disabling base address specifiers (and emitting DWARFv 4) too:
With base addresses (default): Total File Size: 527591064
Without base addresses: Total File Size: 513946184 (-2.59 %)
Sep 17 2020
I have here a copy of the table I shared earlier, with a new row "ShouldUseBaseAddress=false". The stats for this row are taken at 57d8acac64b (D86153) with the changes mentioned in my previous comment (disabling base address specifiers).
+------------------------------------------------------------------------------------------- + | 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 | | ShouldUseBaseAddress=false | 14.9 | 35.5 | 55.3 | 105.7 | +---------------------------------+------------+------------------+-----------------+--------+
Ah, OK - so it sounds like we're back down below the size before I added debug_loc base address specifiers? That's good to hear!
Sep 16 2020
The real thing exists over at D82129 (landed).
Thanks for the data!
Sep 15 2020
@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.
Sep 3 2020
I've reverted this for now (485e6db8729) because one of our internal build bot is failing in the same way. I don't think many (any?) of the bots on http://lab.llvm.org:8011/console build the debuginfo-tests project!
Aug 27 2020
Before landing I removed the XFAIL:* from llvm/test/DebugInfo/AArch64/inlined-argument.ll because it passes with this patch.
Aug 26 2020
@djtodoro do you have any more thoughts on this patch? Copying part of my inline comment here for any other reviewers:
Aug 25 2020
A small update to address comments from @djtodoro.
Oops, I didn't include the Phabricator revision line in the commit message.
Hi @djtodoro thanks for taking a look!
Aug 21 2020
+ Rebase
+ Update test for latest yaml2obj with change suggested by @Higuoxing (thank you!).
+ Make the test more interesting, @dblaikie wdyt?
Aug 20 2020
Just a note: This patch moves the InstructionOrdering class added in D86150 out from DbgEntityHistoryCalculator.cpp to DbgEntityHistoryCalculator.h so that we're able to use it in DebugHandlerBase.
Aug 19 2020
Thanks for the review! I'll land the patch alongside the others in the stack when they're good to go.
Hi, sorry for leaving this for so long! I think it will take me a couple of passes to fully understand it, but for now I left some inline nits/questions. Please feel free to leave the nit changes until later revisions.
Thanks @jhenderson. I think this should have also addressed @dblaikie's concerns regarding the test, so I will aim to land this in a couple of days if there are no objections.
Thank you @aprantl.
Aug 18 2020
Aug 14 2020
Thank you so much @Higuoxing, that's very kind!
Thanks @aprantl and @jhenderson.
Aug 12 2020
Aug 11 2020
Hi @dstenb, thanks for taking a look. I've updated the diff to include a test but it feels a little unwieldy. Is there a better way to test this?
Aug 10 2020
Jul 31 2020
Jul 30 2020
Hi @vsk, just a inline question from me. And in addition, I noticed that SourceLevelDebugging specifically states "there can only be one call to llvm.dbg.declare for a given concrete local variable". I wonder if we should update this to say something like "there can only be one call to llvm.dbg.declare for a given fragment of a local variable"?