Page MenuHomePhabricator

vsk (Vedant Kumar)
User

Projects

User Details

User Since
Jul 8 2015, 10:26 AM (184 w, 3 d)

Recent Activity

Thu, Jan 17

vsk added a comment to D56045: [CodeExtractor] Emit lifetime markers around reloads of outputs.

Gentle ping (this isn't urgent, but it seems nice to have).

Thu, Jan 17, 12:32 PM
vsk added a comment to D56865: [MergeFunc] Allow merging identical vararg functions using aliases.
In D56865#1362011, @rnk wrote:

Crazy idea: use musttail (only some targets support it) to make this work. Of course, first, make it correct.

Thu, Jan 17, 11:53 AM
vsk created D56865: [MergeFunc] Allow merging identical vararg functions using aliases.
Thu, Jan 17, 10:13 AM

Wed, Jan 16

vsk created D56815: [FunctionComparator] Consider tail call kinds.
Wed, Jan 16, 3:14 PM
vsk added a comment to D56764: handle invokes like calls for sample profile data.

(Looks reasonable to me. I haven't really touched this code, so it'd help if another review could +1 the change.)

Wed, Jan 16, 2:50 PM
vsk added inline comments to D56764: handle invokes like calls for sample profile data.
Wed, Jan 16, 12:17 PM

Fri, Jan 11

vsk added a comment to D56587: Fix sign/zero extension in Dwarf expressions..

Thanks for the patch.

Fri, Jan 11, 9:16 AM · debug-info

Thu, Jan 10

vsk created D56574: [MergeFunc] Erase unused duplicate functions if they are discardable.
Thu, Jan 10, 7:01 PM

Mon, Jan 7

vsk added a comment to D56425: [BreakpointList] Simplify/modernize BreakpointList (NFC).

Looks like a nice/reasonable cleanup, thanks!

Mon, Jan 7, 9:04 PM
vsk updated subscribers of D56045: [CodeExtractor] Emit lifetime markers around reloads of outputs.

+ @gyiu, as I believe he's worked on the partial inliner.

Mon, Jan 7, 2:40 PM

Thu, Jan 3

vsk added a comment to D55967: [CodeExtractor] Do not extract unsafe lifetime markers.

Ping.

Thu, Jan 3, 3:20 PM

Mon, Dec 31

vsk added a comment to D56151: [DebugInfo] PR40010: Avoid register coalesing altering DBG_VALUE valuations.

Thanks for working on this.

Mon, Dec 31, 2:33 PM
vsk added inline comments to D55987: [CodeGen] Skip over dbg-instr in twoaddr pass.
Mon, Dec 31, 11:32 AM

Fri, Dec 21

vsk updated the diff for D56045: [CodeExtractor] Emit lifetime markers around reloads of outputs.
  • Tighten up the tests a bit.
Fri, Dec 21, 8:43 PM
vsk added a child revision for D55967: [CodeExtractor] Do not extract unsafe lifetime markers: D56045: [CodeExtractor] Emit lifetime markers around reloads of outputs.
Fri, Dec 21, 8:39 PM
vsk created D56045: [CodeExtractor] Emit lifetime markers around reloads of outputs.
Fri, Dec 21, 8:39 PM
vsk added a comment to D55997: Add support for LLVM profile for NetBSD.

Thanks, this patch lgtm.

Fri, Dec 21, 8:36 PM · Restricted Project
vsk added a comment to D55987: [CodeGen] Skip over dbg-instr in twoaddr pass.

Thanks for the patch!

Fri, Dec 21, 8:34 PM
vsk added a reviewer for D55987: [CodeGen] Skip over dbg-instr in twoaddr pass: debug-info.
Fri, Dec 21, 1:58 PM
vsk added inline comments to D55967: [CodeExtractor] Do not extract unsafe lifetime markers.
Fri, Dec 21, 12:34 PM
vsk updated the diff for D55967: [CodeExtractor] Do not extract unsafe lifetime markers.
Fri, Dec 21, 12:34 PM
vsk created D56019: [IR] Add Instruction::isLifetimeStartOrEnd, NFC.
Fri, Dec 21, 12:16 PM
vsk planned changes to D55967: [CodeExtractor] Do not extract unsafe lifetime markers.

(Marking this WIP while I address @davidxl 's feedback.)

Fri, Dec 21, 11:57 AM

Dec 20 2018

vsk added inline comments to D55967: [CodeExtractor] Do not extract unsafe lifetime markers.
Dec 20 2018, 5:45 PM
vsk created D55967: [CodeExtractor] Do not extract unsafe lifetime markers.
Dec 20 2018, 3:45 PM

Dec 18 2018

vsk updated the diff for D55843: [CodeGen] Handle mixed-width ops in mixed-sign mul-with-overflow lowering.
  • Simplify logic per Eli's review feedback.
Dec 18 2018, 12:06 PM
vsk created D55843: [CodeGen] Handle mixed-width ops in mixed-sign mul-with-overflow lowering.
Dec 18 2018, 11:08 AM

Dec 17 2018

vsk added a comment to D55760: [ADT] IntervalMap: add contains(a, b) method.

I think it'll be really useful to have this helper around. Thanks, lgtm with the name change!

Dec 17 2018, 9:36 AM
vsk accepted D55761: lldb-test ir-memory-map: Use IntervalMap::contains.

Thanks, lgtm!

Dec 17 2018, 9:10 AM
vsk added inline comments to D55760: [ADT] IntervalMap: add contains(a, b) method.
Dec 17 2018, 9:08 AM

Dec 15 2018

vsk added a comment to D51813: [Util] Refer to [s|z]exts of args when converting dbg.declares (fix PR35400).

@aprantl It looks like the percentage of variables with location went up right after this patch landed:

Dec 15 2018, 11:35 AM · debug-info
vsk added a comment to D55658: Avoid Decl::getASTContext in hot paths where possible, Part 1.
In D55658#1332240, @vsk wrote:

Thanks for working on this :). It’d be interesting to see some pre/post patch compile time numbers on CTMark.

Naive/strawman alternative here, but: what’s the impact on peak RSS and compile time of just storing an ASTContext pointer in Decl/DeclContext? If it’s not out of the question to grow Decl etc. and it gives a nice speed up, that might be a simpler approach.

I will try to get some numbers from CTMark.

I don't think that storing a ref/pointer to the ASTContext in each DeclContext (no point in storing it in Decl) is a great idea.
I experimented with this and the speedup from avoiding the lookup of the ASTContext is almost completely offset by
the slowdown from the increased memory usage.

Dec 15 2018, 11:02 AM · Restricted Project
vsk added a comment to D55658: Avoid Decl::getASTContext in hot paths where possible, Part 1.

Thanks for working on this :). It’d be interesting to see some pre/post patch compile time numbers on CTMark.

Dec 15 2018, 10:32 AM · Restricted Project

Dec 14 2018

vsk added a comment to D51813: [Util] Refer to [s|z]exts of args when converting dbg.declares (fix PR35400).
Dec 14 2018, 4:09 PM · debug-info

Dec 13 2018

vsk added a comment to D51813: [Util] Refer to [s|z]exts of args when converting dbg.declares (fix PR35400).

I think this patch is complete, it just needs review.

Dec 13 2018, 10:33 AM · debug-info
vsk updated subscribers of D51813: [Util] Refer to [s|z]exts of args when converting dbg.declares (fix PR35400).

Apologies, I haven't been able to get back to this. I'd be happy to have someone commandeer this.

Dec 13 2018, 10:21 AM · debug-info

Dec 11 2018

vsk added a comment to D55574: Remove else statements after returns.

+ 1 to this. If there's a tidy plugin for misleading indention, that might address some of Adrian's concerns.

Dec 11 2018, 2:54 PM

Dec 7 2018

vsk accepted D55227: [DebugInfo] Don't drop dbg.value's of nullptr.

Looks great, thanks.

Dec 7 2018, 8:53 AM
vsk accepted D55272: [DebugInfo] Remove now un-necessary logic from HoistThenElseCodeToIf.

LGTM. @CarlosAlbertoEnciso, any thoughts on this?

Dec 7 2018, 8:50 AM
vsk accepted D55372: [DebugInfo] Emit "undef" DBG_VALUEs when SDNodes are optimised out.

LGTM, thanks! I'd suggest running through debuginfo-tests if you haven't already (ninja check-debuginfo).

Dec 7 2018, 8:47 AM

Dec 6 2018

vsk added a comment to D55396: [DebugInfo] Make sure CodeGenPrepare does not drop MD references to locals..

Thanks for the patch, this is a great catch.

Dec 6 2018, 4:38 PM · debug-info

Dec 3 2018

vsk added a reviewer for D53887: [HotColdSplitting] Outline more than once per function: kachkov98.
Dec 3 2018, 5:13 PM
vsk updated the diff for D53887: [HotColdSplitting] Outline more than once per function.

Friendly ping. I've rebased this on top of r348205, which fixes the assertion failure pointed out in llvm.org/PR39564.

Dec 3 2018, 5:12 PM
vsk abandoned D54189: [HotColdSplitting] Ensure PHIs have unique incoming values.

I'll abandon this in favor of D55018 and fold some of the tests from this patch into D53887.

Dec 3 2018, 1:16 PM
vsk accepted D55018: [CodeExtractor] Split PHI nodes with incoming values from outlined region (PR39433).

Thank you, LGTM.

Dec 3 2018, 12:46 PM
vsk added inline comments to D55227: [DebugInfo] Don't drop dbg.value's of nullptr.
Dec 3 2018, 11:57 AM

Nov 30 2018

vsk added inline comments to D55151: [gcov/Darwin] Ensure external symbols are exported when using an export list.
Nov 30 2018, 5:07 PM
vsk created D55151: [gcov/Darwin] Ensure external symbols are exported when using an export list.
Nov 30 2018, 4:16 PM
vsk added a comment to D55091: Add --analyze option to llvm-dwarfdump.
In D55091#1314919, @vsk wrote:

Your DWARF analysis tool presumably needs to support diffing and output serialization. So would a comprehensive code size analysis tool. There's no reason why these two tools should have different diffing / serialization options. The latter shouldn't live within a debug info specific tool. However, there's no fundamental issue with adding DWARF to the long list of formats llvm-objdump already understands. Technically, it's just a matter of linking in llvm's DWARF libraries.

Diffing? Of what? If we're tracking sizes etc over time, the diffing shouldn't be in the tool. Dump stats to a database and do the normal thing with it there.

Nov 30 2018, 3:07 PM
vsk added a comment to D55091: Add --analyze option to llvm-dwarfdump.

I feel like llvm-dwarfdump is a great place for analyzing DWARF specific information. I did only DWARF section information in llvm-dwarfdump since that is what the tool concentrates on. If llvm-objdump is already correctly identifying information as debug info, I follow David's train of thought where a user might try to figure out which tool can tell me more about debug info and that would lead to llvm-dwarfdump.

All of the rest of the information that will be added under the --analyze is DWARF specific and might not make as much sense in llvm-objdump. Though we could put the data gathering for the analysis in the DebugInfo/DWARF folder and allow both tools to report the same things if needed.

Nov 30 2018, 10:09 AM

Nov 29 2018

vsk added a comment to D55091: Add --analyze option to llvm-dwarfdump.
In D55091#1313812, @vsk wrote:

What was the end result of the discussion around this and the other size analysis proposal (for objdump?) that came around the same time - and the overlap with Bloaty, for instance?

Reading the thread again (http://lists.llvm.org/pipermail/llvm-dev/2018-September/126501.html), istm that most folks were ok with having a size analysis tool in llvm. I pointed out some specific advantages (code reuse, shared maintenance) of having such a tool live in-tree. David suggested folding bloaty under the llvm umbrella -- my concern (and IIRC @jakehehrlich's as well) was that doing so would amount to a full re-write. I've since switched teams to focus on outliner work, and haven't had time to whip my initial prototype into shape for upstreaming.

Would be good to have a goal - for instance, one of the things about binary size analysis is also looking at the reloc sizes for the debug info sections (& ultimately, probably looking at the whole object file/executable - including headers, etc, not just the section sizes themselves - for comparison purposes (optimizing debug info size only to realize there's something else holding up the object size tent would be unfortunate)).

Agreed, I actually think this would make sense as part of a more comprehensive analysis mode in llvm-objdump.

Do we want to split this sort of functionality between the two tools? I feel like that'd easily lead to people missing important things like the above example.

Nov 29 2018, 6:06 PM
vsk updated subscribers of D55091: Add --analyze option to llvm-dwarfdump.

What was the end result of the discussion around this and the other size analysis proposal (for objdump?) that came around the same time - and the overlap with Bloaty, for instance?

Nov 29 2018, 5:19 PM
vsk added inline comments to D55018: [CodeExtractor] Split PHI nodes with incoming values from outlined region (PR39433).
Nov 29 2018, 2:23 PM
vsk added inline comments to D55018: [CodeExtractor] Split PHI nodes with incoming values from outlined region (PR39433).
Nov 29 2018, 2:05 PM
vsk added a comment to D54189: [HotColdSplitting] Ensure PHIs have unique incoming values.

While this patch is correct, it might make sense to abandon it in favor of https://reviews.llvm.org/D55018, which solves the underlying issue more comprehensively.

Nov 29 2018, 9:58 AM

Nov 28 2018

vsk added inline comments to D55018: [CodeExtractor] Split PHI nodes with incoming values from outlined region (PR39433).
Nov 28 2018, 4:05 PM
vsk added reviewers for D55018: [CodeExtractor] Split PHI nodes with incoming values from outlined region (PR39433): fhahn, davidxl, sebpop, brzycki.

Thank you (very much!) for working on this.

Nov 28 2018, 1:58 PM

Nov 27 2018

vsk updated the diff for D54189: [HotColdSplitting] Ensure PHIs have unique incoming values.
  • Use an iterative approach to remove blocks from the outlining region, if they would cause PHIs outside of the region to become invalid (i.e. have more than one distinct incoming value from the codeRepl block).
Nov 27 2018, 10:34 AM

Nov 26 2018

vsk added a comment to D54195: Fix linker option for -fprofile-arcs -ftest-coverage.

@jessicah Please ping this review if you need someone to commit this patch on your behalf.

Nov 26 2018, 11:19 AM
vsk added inline comments to D54175: [PGO] context sensitive PGO.
Nov 26 2018, 11:07 AM

Nov 20 2018

vsk updated subscribers of D54731: [lit] Enable the use of custom user-defined lit commands.

For compiling/linking, I think we can get by using lit substitutions to fill in platform-specific options? iOS testing for Swift is done this way (both on-device and simulator), as is testing for the profiling runtime. Dan and @filcab are more active in the area of sanitizer runtime testing, so they may have more informed opinions to share about how well that model works.

Nov 20 2018, 6:31 PM

Nov 18 2018

vsk created D54686: [IR] Add hasNPredecessors, hasNPredecessorsOrMore to BasicBlock.
Nov 18 2018, 11:37 PM
vsk added a comment to D40477: Enable Partial Inlining by default.

Imho CodeExtractor needs a little more work before it's ready to be on-by-default. There are two main blockers: missing debug info support, and a missing whitelist of extract-able intrinsics. The latter poses a high risk for miscompiles (see llvm.org/PR39545, llvm.org/PR39671).

Nov 18 2018, 9:19 PM
vsk added a comment to D53244: [Coverage] Fix PR39258: support coverage regions that start deeper than they end.

Friendly ping -- @orivej were you still looking for more feedback? If not, do you still need someone to land this patch on your behalf?

Nov 18 2018, 9:06 PM
vsk accepted D53231: [Sema] Fix PR38987: keep end location of a direct initializer list.

The history seems complicated. I think it'd be really useful to sort out why getParenOrBraceRange() couldn't give the right result, but I'd be happy to see this land first to address the crash.

Nov 18 2018, 9:05 PM
vsk accepted D54669: [ProfileSummary] Standardize methods and fix comment..

I agree that this looks a bit more consistent with the rest of the codebase. LGTM, thanks.

Nov 18 2018, 8:42 AM

Nov 16 2018

Herald updated subscribers of D40477: Enable Partial Inlining by default.
Nov 16 2018, 11:41 PM

Nov 15 2018

vsk added inline comments to D54599: [Profile] Avoid race condition when dumping GCDA files..
Nov 15 2018, 2:52 PM
vsk added a comment to D54599: [Profile] Avoid race condition when dumping GCDA files..

Could you share either a link to or the abbreviated text of the test failure?

Nov 15 2018, 2:01 PM

Nov 14 2018

vsk planned changes to D54189: [HotColdSplitting] Ensure PHIs have unique incoming values.

Actually, when I defined “IsCold” as “has more than one predecessor” to stress-test the pass, I found another instance of this assert firing while building a stage2 clang.

Nov 14 2018, 4:10 PM
vsk added a comment to D54189: [HotColdSplitting] Ensure PHIs have unique incoming values.

Friendly ping.

Nov 14 2018, 12:22 PM
vsk added a comment to D53887: [HotColdSplitting] Outline more than once per function.

Ping, are there any outstanding concerns about this one? It'd be nice to have this in-tree, as I have a few follow-ups based on it.

Nov 14 2018, 12:20 PM
vsk added inline comments to D54244: [HotColdSplitting] Refine definition of unlikelyExecuted.
Nov 14 2018, 12:14 PM
vsk updated the diff for D54244: [HotColdSplitting] Refine definition of unlikelyExecuted.
  • Handle cold invokes, and address review feedback from @junbuml
Nov 14 2018, 12:14 PM

Nov 13 2018

vsk added inline comments to D54175: [PGO] context sensitive PGO.
Nov 13 2018, 10:22 PM

Nov 10 2018

vsk accepted D54385: Remove comments after header includes..

Thanks, lgtm.

Nov 10 2018, 4:22 PM · Restricted Project

Nov 9 2018

vsk updated the diff for D54244: [HotColdSplitting] Refine definition of unlikelyExecuted.
  • Split out the change to mark llvm.trap cold, per review feedback.
Nov 9 2018, 10:46 AM
vsk created D54329: Mark @llvm.trap cold.
Nov 9 2018, 9:46 AM

Nov 8 2018

vsk added a comment to D54217: Add total function byte size and inline function byte size to "llvm-dwarfdump --statistics".

Why not check in a .o? The numbers won't change, so they can meaningfully be compared.

Nov 8 2018, 5:04 PM
vsk accepted D54266: [llvm-cov] Add lcov tracefile export format..

Thanks.

Nov 8 2018, 3:38 PM
vsk added a comment to D54217: Add total function byte size and inline function byte size to "llvm-dwarfdump --statistics".

This looks good to me. I think it'd help to have a +1 from Adrian, as he's worked on this file much more than I have.

Nov 8 2018, 2:56 PM
vsk added inline comments to D54266: [llvm-cov] Add lcov tracefile export format..
Nov 8 2018, 12:58 PM
vsk added a comment to D54266: [llvm-cov] Add lcov tracefile export format..

Oh, I just realized this also needs an entry in CommandGuide (I think the file is called llvm-cov.rst).

Nov 8 2018, 12:56 PM
vsk added a comment to D54195: Fix linker option for -fprofile-arcs -ftest-coverage.

Thanks for the patch. Would you mind uploading a diff with additional context (e.g. git diff -U10000)? Phab doesn't render it.

Nov 8 2018, 12:01 PM
vsk added reviewers for D54195: Fix linker option for -fprofile-arcs -ftest-coverage: calixte, marco-c, sylvestre.ledru.
Nov 8 2018, 11:59 AM
vsk updated subscribers of D54175: [PGO] context sensitive PGO.

Hi Rong, at a high-level, I like what this patch is doing. I'll try to leave in-depth comments by next week -- please ping the review otherwise.

Nov 8 2018, 11:16 AM
vsk accepted D54266: [llvm-cov] Add lcov tracefile export format..

I suggest adding a note in docs/ReleaseNotes.rst about this.

Nov 8 2018, 11:02 AM

Nov 7 2018

vsk added a comment to D54244: [HotColdSplitting] Refine definition of unlikelyExecuted.

This is motivated by functions like exit() and longjmp(), which are not beneficial to outline.

exit() seems like a weird example: it executes at most once per process. What's the downside of outlining it?

Nov 7 2018, 5:39 PM
vsk updated the summary of D54244: [HotColdSplitting] Refine definition of unlikelyExecuted.
Nov 7 2018, 5:14 PM
vsk created D54244: [HotColdSplitting] Refine definition of unlikelyExecuted.
Nov 7 2018, 5:14 PM
vsk added a comment to D54217: Add total function byte size and inline function byte size to "llvm-dwarfdump --statistics".

Looks reasonable to me, but it'd help to check in a .o for a test.

Nov 7 2018, 5:14 PM
vsk accepted D52034: [Clang] Add options -fprofile-filter-files and -fprofile-exclude-files to filter the files to instrument with gcov.
Nov 7 2018, 3:23 PM
vsk updated the diff for D54189: [HotColdSplitting] Ensure PHIs have unique incoming values.
  • Do not add duplicate blocks to the outlining region. The test for this is 'forward-dfs-reaches-marked-block.ll'.
Nov 7 2018, 2:48 PM
vsk added a comment to D53887: [HotColdSplitting] Outline more than once per function.
In D53887#1290587, @vsk wrote:

I guess it would be good to give some stress test on this pass to see if there is any hidden bug by relaxing conditions in unlikelyExecuted(). As we treat more blocks as cold without being limited on unlikelyExecuted(), we maybe able to expose hidden issues with it.

That's a great idea. I'll do that now with the test-suite + externals. FWIW, I also built all of iOS with this pass enabled, and the only compiler crash I found was https://bugs.llvm.org/show_bug.cgi?id=39564. I'm still investigating possible miscompiles.

Nov 7 2018, 2:36 PM
vsk added a comment to D54217: Add total function byte size and inline function byte size to "llvm-dwarfdump --statistics".
In D54217#1290504, @vsk wrote:

Thanks. There seems to be some overlap with

[llvm-dev] RFC: Adding a code size analysis tool
https://lists.llvm.org/pipermail/llvm-dev/2018-September/126501.html

how was that discussion resolved?

I think the response was generally positive but I haven't had the time to push things into tree. @clayborg, have you tried out the tool here: https://github.com/vedantk/llvm-project ? The RFC has some usage instructions.

If you just want the ratio of inlined to not-inlined bytes, this patch is a simpler way to do it. But if you need something more fine-grained (i.e. *which* functions were inlined more compared to a baseline, by how much, etc.), it might be worth checking out. Happy to have someone else run with the code.

This current patch is indeed for an overview of inlined to not-inlined bytes. I have other patches to --statistics that will display more output coming. Breaking down exact sizes of inlined functions by count and size will be part of this future output. But this current one is just for a quick breakdown. Like you said in the URL provided, it is nice to know that say 30% of your code is inlined. If you are expecting less inlining and try a new option, this can be a quick way to see results in a very concise kind of way.

Nov 7 2018, 1:29 PM
vsk added a comment to D53887: [HotColdSplitting] Outline more than once per function.

I guess it would be good to give some stress test on this pass to see if there is any hidden bug by relaxing conditions in unlikelyExecuted(). As we treat more blocks as cold without being limited on unlikelyExecuted(), we maybe able to expose hidden issues with it.

Nov 7 2018, 1:00 PM
vsk added a comment to D53887: [HotColdSplitting] Outline more than once per function.

IMHO the right fix is to not treat noreturn calls as cold (also as a follow-up).

I'm not sure if handling noreturn is a right fix. A block containing exit(0) will have "unreachable", so it must be still considered as a cold block even after removing noreturn from unlikelyExecuted().

Nov 7 2018, 12:55 PM
vsk updated subscribers of D54217: Add total function byte size and inline function byte size to "llvm-dwarfdump --statistics".

Thanks. There seems to be some overlap with

[llvm-dev] RFC: Adding a code size analysis tool
https://lists.llvm.org/pipermail/llvm-dev/2018-September/126501.html

how was that discussion resolved?

Nov 7 2018, 11:59 AM
vsk added inline comments to D54189: [HotColdSplitting] Ensure PHIs have unique incoming values.
Nov 7 2018, 11:52 AM
vsk updated the diff for D54189: [HotColdSplitting] Ensure PHIs have unique incoming values.
  • Address review feedback from Aditya.
Nov 7 2018, 11:52 AM