vsk (Vedant Kumar)
User

Projects

User Details

User Since
Jul 8 2015, 10:26 AM (153 w, 5 d)

Recent Activity

Today

vsk created D48305: [IR] Introduce helpers to skip meta-instructions.
Mon, Jun 18, 4:57 PM
vsk added inline comments to D47874: [SCEVExp] Advance found insertion point until we find a non-dbg instruction..
Mon, Jun 18, 2:18 PM

Sat, Jun 9

vsk added a comment to D47973: [ADT] Change the behavior of `StringRef::rsplit()` when the separator is not found..

Thanks for the patch. The StringRef change itself looks fine, but please make sure to update the callsites in-tree (e.g in llvm, clang, lldb etc).

Sat, Jun 9, 5:17 AM

Fri, Jun 8

vsk added a comment to D45637: [DebugInfo] Ignore DBG_VALUE instructions in PostRA Machine Sink.

I'd like to see this move forward. I'll be out of the office over the next week. I can try to help find another reviewer when I'm back.

Fri, Jun 8, 5:26 PM · debug-info
vsk added a comment to D47874: [SCEVExp] Advance found insertion point until we find a non-dbg instruction..

Or maybe it would be even more convenient to just have a new iterator type, which skips such meta instructions.

Fri, Jun 8, 3:11 PM
vsk added a comment to D47874: [SCEVExp] Advance found insertion point until we find a non-dbg instruction..

Not sure this is the best fix. The function is named findInsertPointAfter; from the name, I would expect it to behave similarly to BasicBlock::getFirstInsertionPt (which does not try to move past debug info).

We added instructionsWithoutDebug recently to deal with this sort of issue, but it's not directly usable here.

Yeah, maybe we should have a function that takes an iterator and advances the iterator to the first non debug/meta instruction?

Fri, Jun 8, 1:15 PM

Thu, Jun 7

vsk accepted D47406: [ADT] Add `StringRef::rsplit(StringRef Separator)`..
In D47406#1125104, @MTC wrote:

rsplit(char) already exists. That said, another way to reuse code would be
to have rsplit(char) call the StringRef version

Thanks for your advice, @zturner! I have refactored split(char) and rsplit(char) to use StringRef version. I think the code is more clean now.

Thu, Jun 7, 1:31 PM
vsk updated subscribers of D47874: [SCEVExp] Advance found insertion point until we find a non-dbg instruction..

Thanks for doing this. Do you think we have the same bug here w.r.t lifetime markers, assume intrinsics, etc? If so, maybe this patch would be the right vehicle to add something like Instruction::isMetaInstruction, as suggested by Hideki in a previous thread.

Thu, Jun 7, 8:40 AM

Tue, Jun 5

vsk accepted D47720: [DebugInfo] Inline for without DebugLocation.

As is the case with D47097, this helps preserve more scope information after SROA. LGTM. Thanks!

Tue, Jun 5, 1:59 PM

Mon, Jun 4

vsk updated subscribers of D46815: [DbgInfo] Fix StripDebugInfo.

If you have spare cycles, could you try to take a look at debug value loss in instcombine? I started looking at this issue circa r323570 but got sidetracked with other work.

Mon, Jun 4, 4:55 PM
vsk updated subscribers of D46815: [DbgInfo] Fix StripDebugInfo.

Opt's -strip-dead-prototypes mode does exactly this.

Mon, Jun 4, 10:32 AM
vsk added a comment to D46815: [DbgInfo] Fix StripDebugInfo.

I think we can close this, then. The functionality needed to test debug info invariance is already in tree. Take a look at https://reviews.llvm.org/rL333861.

Mon, Jun 4, 10:11 AM
vsk added reviewers for D47371: [BPI] Apply invoke heuristic before loop branch heuristic: davidxl, wmi.

I think this looks good, but I'm not familiar enough with BPI to give an official lgtm.

Mon, Jun 4, 10:09 AM

Sun, Jun 3

vsk requested changes to D47406: [ADT] Add `StringRef::rsplit(StringRef Separator)`..

Hi @MTC, thanks for the patch.

Sun, Jun 3, 5:28 PM
vsk added a comment to D46815: [DbgInfo] Fix StripDebugInfo.

Does opt -strip-dead-prototypes handle all of this?

Sun, Jun 3, 3:56 PM

Fri, Jun 1

vsk created D47663: Add a debug dump for DbgValueHistoryMap.
Fri, Jun 1, 3:22 PM
vsk added a comment to D47646: [IRMemoryMap] Use labels in the "malloc" and "free" lldb-test commands.

I can look into the failure - but can you XFAIL the test rather than skipping it and log a bug, so that we can track the failure rather than potentially assuming down the line that the test is not meant for windows?

Fri, Jun 1, 1:37 PM
vsk added a comment to D47646: [IRMemoryMap] Use labels in the "malloc" and "free" lldb-test commands.
In D47646#1119487, @vsk wrote:
In D47646#1119471, @vsk wrote:

While you are at it, can you make sure this works on Windows? The current version of the test that is checked in fails.

Sorry about that. Could you point me to the error message?

"Cannot use process to test IRMemoryMap"

Hm, I'm afraid this just tells us that the debugger either failed to create a process, or that the process is not alive / incapable of JIT'ing. As I don't have a Windows machine on hand, I can't dig into this further. Would you mind taking a look? I can simply disable these tests on Windows for now.

Fri, Jun 1, 1:03 PM
vsk added a comment to D47646: [IRMemoryMap] Use labels in the "malloc" and "free" lldb-test commands.
In D47646#1119471, @vsk wrote:

While you are at it, can you make sure this works on Windows? The current version of the test that is checked in fails.

Sorry about that. Could you point me to the error message?

"Cannot use process to test IRMemoryMap"

Fri, Jun 1, 1:02 PM
vsk added a comment to D47646: [IRMemoryMap] Use labels in the "malloc" and "free" lldb-test commands.

While you are at it, can you make sure this works on Windows? The current version of the test that is checked in fails.

Fri, Jun 1, 12:43 PM
vsk created D47646: [IRMemoryMap] Use labels in the "malloc" and "free" lldb-test commands.
Fri, Jun 1, 11:36 AM

Thu, May 31

vsk updated the diff for D47551: [IRMemoryMap] Fix the alignment adjustment in Malloc.
  • Address Pavel's feedback, remove a questionable FIXME.
Thu, May 31, 2:02 PM
vsk added a comment to D47551: [IRMemoryMap] Fix the alignment adjustment in Malloc.

LGTM.

I haven't looked at process memory management. How hard would your FIXME be to implement?

Thu, May 31, 1:58 PM

Wed, May 30

vsk updated the diff for D47551: [IRMemoryMap] Fix the alignment adjustment in Malloc.
  • Don't insert extra padding bytes when alignment = 1.
  • + Lang
Wed, May 30, 2:11 PM
vsk created D47551: [IRMemoryMap] Fix the alignment adjustment in Malloc.
Wed, May 30, 12:41 PM
vsk added a dependent revision for D47508: [lldb-test] Add a testing harness for the JIT's IRMemoryMap: D47551: [IRMemoryMap] Fix the alignment adjustment in Malloc.
Wed, May 30, 12:41 PM
vsk updated the summary of D47551: [IRMemoryMap] Fix the alignment adjustment in Malloc.
Wed, May 30, 12:41 PM
vsk updated the diff for D47508: [lldb-test] Add a testing harness for the JIT's IRMemoryMap.
  • Really fix the allocation overlap test. The previous version of this patch would not detect overlaps in which the end of the new allocation is contained within an existing allocation.
Wed, May 30, 11:55 AM
vsk added inline comments to D47508: [lldb-test] Add a testing harness for the JIT's IRMemoryMap.
Wed, May 30, 10:43 AM
vsk updated the diff for D47508: [lldb-test] Add a testing harness for the JIT's IRMemoryMap.
  • Use %zu, and improve detection of overlapping allocations.
Wed, May 30, 10:43 AM

Tue, May 29

vsk updated the summary of D47508: [lldb-test] Add a testing harness for the JIT's IRMemoryMap.
Tue, May 29, 6:21 PM
vsk created D47508: [lldb-test] Add a testing harness for the JIT's IRMemoryMap.
Tue, May 29, 6:20 PM

Fri, May 25

vsk added a comment to D46918: [Coverage] Discard the last uncompleted deferred region in a decl.

Ping.

Fri, May 25, 1:11 PM
vsk added a comment to D47371: [BPI] Apply invoke heuristic before loop branch heuristic.

Thanks for the patch. Full disclaimer: I don't have much knowledge about this part of the codebase.

Fri, May 25, 10:16 AM

Thu, May 24

vsk added a comment to D47237: [DSE] Calloc-strlen optimizations.
In D47237#1109008, @vsk wrote:

Is this a common pattern, or are there real-world benchmarks this improves?

This adds a fair bit of code to a hot part of the compiler, so it'd be worth sharing LNT and CTMark data to make sure there aren't any regressions (http://llvm.org/docs/lnt/quickstart.html, https://github.com/llvm-mirror/test-suite/tree/master/CTMark).

I will try to run that benchmarks, but I see no reasons for regressions, but I will check it :)

Thu, May 24, 4:49 PM
vsk accepted D47347: [DebugInfo] Maintain DI when converting GEP to bitcast.

LGTM. Thanks!

Thu, May 24, 3:16 PM · debug-info
vsk requested changes to D46815: [DbgInfo] Fix StripDebugInfo.
Thu, May 24, 11:09 AM
vsk added inline comments to D46815: [DbgInfo] Fix StripDebugInfo.
Thu, May 24, 11:08 AM
vsk added a comment to D47097: [DebugInfo] Preserve scope in auto generated StoreInst.

Can we step back a second and better explain what the problem is? With current Clang the debug info for this function looks okay to me.
The store that is "missing" a debug location is homing the formal parameter to its local stack location; this is part of prolog setup, not "real" code.

Thu, May 24, 11:00 AM

Wed, May 23

vsk accepted D47282: [DebugInfo] Maintain DI for sunken bitcasts.

LGTM with a nitpick. Thanks!

Wed, May 23, 2:46 PM · debug-info
vsk added inline comments to D47097: [DebugInfo] Preserve scope in auto generated StoreInst.
Wed, May 23, 1:19 PM
vsk added inline comments to D47097: [DebugInfo] Preserve scope in auto generated StoreInst.
Wed, May 23, 1:01 PM
vsk added inline comments to D47097: [DebugInfo] Preserve scope in auto generated StoreInst.
Wed, May 23, 10:40 AM

Tue, May 22

vsk added a comment to D47237: [DSE] Calloc-strlen optimizations.

Is this a common pattern, or are there real-world benchmarks this improves?

Tue, May 22, 8:36 PM
vsk accepted D47231: [Coverage] Update CSS to make HTML reports copy-paste friendly..

LGTM. Thanks!

Tue, May 22, 3:07 PM
vsk added a comment to D47208: [profile] Support profiling runtime on Fuchsia.

This looks good at a high level. How is it tested?

Tue, May 22, 10:58 AM
vsk added inline comments to D47097: [DebugInfo] Preserve scope in auto generated StoreInst.
Tue, May 22, 10:01 AM

Mon, May 21

vsk added inline comments to D47097: [DebugInfo] Preserve scope in auto generated StoreInst.
Mon, May 21, 2:48 PM
vsk added a reviewer for D47097: [DebugInfo] Preserve scope in auto generated StoreInst: aprantl.
Mon, May 21, 12:49 PM
vsk added inline comments to D47097: [DebugInfo] Preserve scope in auto generated StoreInst.
Mon, May 21, 12:46 PM
vsk added a comment to D47097: [DebugInfo] Preserve scope in auto generated StoreInst.

I think CodeGenFunction::EmitParmDecl is the right place to set up an ApplyDebugLocation instance. You can look at CodeGenFunction::EmitAutoVarInit for an example of how to use ApplyDebugLocation.

Mon, May 21, 9:46 AM

May 18 2018

vsk accepted D46815: [DbgInfo] Fix StripDebugInfo.

LGTM. Thanks! Would you like me to commit this for you?

May 18 2018, 11:06 AM
vsk added inline comments to D46815: [DbgInfo] Fix StripDebugInfo.
May 18 2018, 9:12 AM

May 17 2018

vsk updated the diff for D46918: [Coverage] Discard the last uncompleted deferred region in a decl.
  • Add a regression test for switches.
May 17 2018, 11:16 AM
vsk added a comment to D46941: [Debugify] Print errors and warnings to stderr.

Thanks for cleaning up the tests further. I think it's worthwhile to not have run lines which clutter up the output because it slows down the llvm-lit -a -Dopt="opt -debugify ..." workflow.

May 17 2018, 11:08 AM
vsk accepted D46941: [Debugify] Print errors and warnings to stderr.

LGTM, thanks!

May 17 2018, 6:34 AM

May 16 2018

vsk added a comment to D46985: [NFC] WebAssembly build break #2.

IIUC the dump method is still available via libObject, which should be sufficient?

May 16 2018, 3:40 PM
vsk accepted D46985: [NFC] WebAssembly build break #2.

LGTM, I've confirmed this fixes my local modules build.

May 16 2018, 3:34 PM
vsk added a comment to D46977: [NFC] WebAssembly build fix.

I still see a build break with LLVM_ENABLE_MODULES=On:

May 16 2018, 3:25 PM
vsk added a comment to D46847: [WebAssembly] Move toString helpers to BinaryFormat. NFC..

After rebasing on r332530, the fix I suggested no longer works. I'll take another look.

May 16 2018, 3:23 PM
vsk created D46976: [STLExtras] Add size() for ranges, and remove distance().
May 16 2018, 2:18 PM
vsk added a comment to D46847: [WebAssembly] Move toString helpers to BinaryFormat. NFC..

I can't reproduce those failures locally. I also build with BUILD_SHARED_LIBS=ON so I would expect to see the same errors as you.

May 16 2018, 1:42 PM
vsk added a comment to D46847: [WebAssembly] Move toString helpers to BinaryFormat. NFC..

I'm seeing a few link failures due to this commit (I think):

May 16 2018, 11:00 AM
vsk added inline comments to D46815: [DbgInfo] Fix StripDebugInfo.
May 16 2018, 9:58 AM
vsk added a comment to D46941: [Debugify] Print errors and warnings to stderr.

Just like llvm-lit I left the PASS or FAIL to stdout.

On second thoughts this breaks the piping as well. Should I move it to the error stream as well?

May 16 2018, 9:01 AM

May 15 2018

vsk created D46918: [Coverage] Discard the last uncompleted deferred region in a decl.
May 15 2018, 5:08 PM
vsk accepted D46908: [Debugfiy][WIP] Print the pass name next to the result.

As a follow-up, would you mind fixing the situation with debugify output? Currently we're dumping a lot of information to stdout. Ideally we would only print the result of the pipeline to stdout, so as to not break piping opt's output to other LLVM tools. As a first step, I think all of debugify's warnings and messages should be sent to stderr, and then eventually to a separate file.

May 15 2018, 4:16 PM
vsk updated subscribers of D46908: [Debugfiy][WIP] Print the pass name next to the result.

Thanks for the patch. Comments inline --

May 15 2018, 3:39 PM
vsk added inline comments to D46815: [DbgInfo] Fix StripDebugInfo.
May 15 2018, 2:36 PM
vsk added a comment to D46815: [DbgInfo] Fix StripDebugInfo.

The test is now simplified. Please tell me if you really want to remove -strip-debugify option and use -strip-debug instead.

May 15 2018, 2:28 PM
vsk added inline comments to D46815: [DbgInfo] Fix StripDebugInfo.
May 15 2018, 11:21 AM

May 14 2018

vsk requested changes to D46815: [DbgInfo] Fix StripDebugInfo.

Thanks for the patch.

May 14 2018, 5:39 PM
vsk added a comment to D46694: [diagtool] Install diagtool.

Sgtm. I think it would be worthwhile to release-note this.

May 14 2018, 11:19 AM

May 12 2018

vsk accepted D46525: [Debugify] Introduce debugify-each and DebugifyFunctionPass.

LGTM. If you don’t have commit access, let me know, and I can commit this for you on Monday.

May 12 2018, 10:46 AM

May 11 2018

vsk added a comment to D46525: [Debugify] Introduce debugify-each and DebugifyFunctionPass.

Addressed review comments.

I am thinking about making this work for BasicBlockPass and LoopPass.

May 11 2018, 4:15 PM
vsk added inline comments to D46525: [Debugify] Introduce debugify-each and DebugifyFunctionPass.
May 11 2018, 11:22 AM
vsk added inline comments to D46525: [Debugify] Introduce debugify-each and DebugifyFunctionPass.
May 11 2018, 11:17 AM
vsk added a comment to D46525: [Debugify] Introduce debugify-each and DebugifyFunctionPass.

Thanks for working on this @tyb0807, it looks really close :). There are just a few more minor things I'd like addressed.

May 11 2018, 11:12 AM

May 10 2018

vsk updated the diff for D46668: [STLExtras] Add distance() for ranges, pred_size(), and succ_size().
  • Add {pred,succ}_size helpers and use getNumUses() as suggested.
May 10 2018, 1:31 PM
vsk added a comment to D46668: [STLExtras] Add distance() for ranges, pred_size(), and succ_size().

I sort of wonder if we should have a pred_size and succ_size helper.

May 10 2018, 11:18 AM

May 9 2018

vsk created D46668: [STLExtras] Add distance() for ranges, pred_size(), and succ_size().
May 9 2018, 5:29 PM
vsk added inline comments to D46635: [SimplifyCFG] Fix a debug invariant bug in FoldBranchToCommonDest().
May 9 2018, 4:29 PM
vsk updated the diff for D46158: [DAGCombiner] Set the right SDLoc on a newly-created sextload.

Ping. I think there was still a question about register allocation changes which result from specifying the right IROrder. Is this something to be addressed in the scheduler, or should I be retaining the old IROrder here?

May 9 2018, 11:50 AM
vsk added a comment to D46525: [Debugify] Introduce debugify-each and DebugifyFunctionPass.

One last point: to aid debugging, it would help if you could dump the Module / Function IR when the check-debugify step fails. Currently, if a pass fails checking, opt simply moves on without printing any helpful resources.

May 9 2018, 11:30 AM
vsk added a comment to D46525: [Debugify] Introduce debugify-each and DebugifyFunctionPass.

Thanks! At a high level this is looking good. I have more comments inline which should help with code reuse. Currently there is some extra duplicated logic for function passes, and this should be consolidated for easier maintenance.

May 9 2018, 11:28 AM

May 8 2018

vsk added a comment to D46568: [SimplifyCFG] Fix a crash when folding PHIs.

This seems sensible to me! It'd be great if someone more familiar with simplifyCfg could +1.

May 8 2018, 4:02 PM
vsk added inline comments to D46568: [SimplifyCFG] Fix a crash when folding PHIs.
May 8 2018, 11:49 AM
vsk accepted D46478: [Coverage] Take filenames into account when loading function records..

LGTM, thanks!

May 8 2018, 11:37 AM
vsk added inline comments to D46478: [Coverage] Take filenames into account when loading function records..
May 8 2018, 10:34 AM

May 7 2018

vsk added a comment to D46478: [Coverage] Take filenames into account when loading function records..
In D46478#1090568, @vsk wrote:
LLVM-Unit :: ProfileData/./ProfileDataTests/ParameterizedCovMapTest/CoverageMappingTest.skip_duplicate_function_record/0

This test was added in r284251 along with the initial support for loading coverage data from multiple object files. The basic idea behind seems reasonable, as it makes sense to skip records which are exact duplicates. Do you think the duplicate detection could be enhanced by taking filenames and function hashes into account?

  • Ah, I see your update to the unit test. Yes, I think a version of loadFunctionRecord which satisfies this test would be great.

Thanks for taking a look. I hope so, but can't figure out a proper way to implement that. Continue looking into that.

May 7 2018, 3:50 PM
vsk added a comment to D46478: [Coverage] Take filenames into account when loading function records..
LLVM-Unit :: ProfileData/./ProfileDataTests/ParameterizedCovMapTest/CoverageMappingTest.skip_duplicate_function_record/0
May 7 2018, 3:40 PM
vsk added a comment to D46403: [CFI] Force LLVM to die if the implicit blacklist files cannot be found..

Do you have commit access? If not I'd be happy to land this for you.

May 7 2018, 1:19 PM
vsk updated subscribers of D45878: [DEBUG INFO] Fixing cases where debug info (-g) causes changes in the program..

FWIW, I hacked up a fuzzer which found already around 20 (possibly with duplicate) changes in generated code due to -g being passed around. (cc: @zhendongsu)

There are lots of minor cases. The .cfi_* directives being a scheduling barrier creates a lot of noise in such testing (PR37240).

It was prodded by the bug Geoff reported on the SLPVectorizer which has been fixed on Friday.
I can try to reduce the issues reported and also try to fix them.

@davide can you coordinate with @vsk and the GSOC intern on these?

May 7 2018, 1:08 PM
vsk added a comment to D46525: [Debugify] Introduce debugify-each and DebugifyFunctionPass.

Thanks for the patch. I have some comments inline.

May 7 2018, 11:38 AM

May 4 2018

vsk accepted D46403: [CFI] Force LLVM to die if the implicit blacklist files cannot be found..
In D46403#1088759, @pcc wrote:

We should be installing compiler-rt/lib/cfi/cfi_blacklist.txt, no?

May 4 2018, 4:48 PM
vsk added a comment to D46478: [Coverage] Take filenames into account when loading function records..

No concerns from me, as we appear to deduplicate function records at a lower level (in VersionedCovMapReader).

May 4 2018, 3:57 PM
vsk added a comment to D46403: [CFI] Force LLVM to die if the implicit blacklist files cannot be found..

One problem with this direction is that clang doesn't ship a cfi blacklist in its default install, so, this leaves cfi users with stock toolchains to fend for themselves. I think it'd be a good idea to ship a basic cfi blacklist in clang's resource dir to avoid inadvertent breakage.

May 4 2018, 12:33 PM
vsk added inline comments to D46403: [CFI] Force LLVM to die if the implicit blacklist files cannot be found..
May 4 2018, 12:27 PM
vsk added inline comments to D46403: [CFI] Force LLVM to die if the implicit blacklist files cannot be found..
May 4 2018, 12:25 PM
vsk accepted D46348: [SelectionDAG] Transfer DbgValues when casts are optimized in SelectionDAG::getNode.

LGTM, thanks!

May 4 2018, 9:55 AM