Page MenuHomePhabricator

vsk (Vedant Kumar)
User

Projects

User Details

User Since
Jul 8 2015, 10:26 AM (272 w, 1 d)

Recent Activity

Yesterday

vsk added inline comments to D85670: [Instruction] Add updateLocationAfterHoist helper.
Wed, Sep 23, 4:52 PM · Restricted Project
vsk updated the diff for D85670: [Instruction] Add updateLocationAfterHoist helper.
  • To drop the location of a call within a function with no subprogram, set an empty DebugLoc instead of trying to preserve scope/inlinedAt info. Update a unit test accordingly.
Wed, Sep 23, 4:48 PM · Restricted Project

Mon, Sep 21

vsk added a comment to D87868: [RFC] When calling the process mmap try to call all found instead of just the first one.

It seems like calling any 'mmap' definition should work. Is the interposed mmap implementation failing and correctly returning -1, or is it succeeding and incorrectly returning -1? In either case, it seems like it's worth digging into why it's failing / returning the wrong result. (Just my two cents - I'm not really familiar with this code, so take it with a grain of salt :)

Mon, Sep 21, 2:57 PM · Restricted Project

Fri, Sep 18

vsk added a comment to D87648: [Coverage][NFC] Remove skipped region after added into MappingRegions.

(Depending on what the potential performance gains look like, it might be advisable to keep things simple with the current O(n^2) approach. Optimizing things can carry some risk -- not sure if we've already ruled this out, but e.g. perhaps there's some edge case with nested functions where we maybe don't want to erase skipped regions eagerly.)

Fri, Sep 18, 2:24 PM · Restricted Project
vsk added a comment to D87648: [Coverage][NFC] Remove skipped region after added into MappingRegions.

Unfortunately it does look like the work done in gatherSkippedRegions is O(n^2) in the number of functions, at the moment. If the goal is to speed it up, it'd be good to grab some performance numbers for some representative compile unit (the sqlite3 amalgamation is my go-to for this sort of thing).

Fri, Sep 18, 2:18 PM · Restricted Project
vsk added a comment to D86974: [IRSim] Adding basic implementation of llvm-sim..

(This is really nice work! FTR, my comments were drive-bys, I didn't intend for them to be blocking.)

Fri, Sep 18, 1:58 PM · Restricted Project
vsk added a comment to D84988: [Coverage] Add empty line regions to SkippedRegions.

@zequanwu FTR, I don't have any outstanding concerns (I understand you might be asking for a second reviewer to chime in though).

Fri, Sep 18, 1:50 PM · Restricted Project, Restricted Project, Restricted Project
vsk committed rG3c731ba5f1b6: [llvm-cov] Allow commas in filenames passed to `-object` flag (authored by vsk).
[llvm-cov] Allow commas in filenames passed to `-object` flag
Fri, Sep 18, 1:46 PM
vsk added a comment to D87003: [llvm-cov] Allow commas in filenames passed to `-object` flag.

@andrewjcg I've committed this in 3c731ba5f1b. I found conflicting information online about whether Windows allows filenames with commas in them. To err on the safe side (and avoid potentially corrupting the git history for some llvm devs), I changed the test. Hope you don't mind -- if you prefer another way of doing it, let me know.

Fri, Sep 18, 1:46 PM · Restricted Project
vsk closed D87003: [llvm-cov] Allow commas in filenames passed to `-object` flag.
Fri, Sep 18, 1:46 PM · Restricted Project

Thu, Sep 17

vsk committed rG4926a5ee6301: [lldb] Clarify docstring for SBBlock::IsInlined, NFC (authored by vsk).
[lldb] Clarify docstring for SBBlock::IsInlined, NFC
Thu, Sep 17, 4:56 PM

Wed, Sep 16

vsk added a comment to D87302: [IRSim][IROutliner] Adding DebugInfo handling for IR outlined functions..

CodeExtractor contains a utility to fix up debug info after extraction occurs (I believe it's called fixupDebugInfoPostExtraction). It's solving the same problem - is there any way to generalize/reuse that logic?

Wed, Sep 16, 9:58 AM · debug-info, Restricted Project
vsk added inline comments to D86974: [IRSim] Adding basic implementation of llvm-sim..
Wed, Sep 16, 9:50 AM · Restricted Project
vsk added inline comments to D86974: [IRSim] Adding basic implementation of llvm-sim..
Wed, Sep 16, 9:48 AM · Restricted Project

Mon, Sep 14

vsk accepted D87003: [llvm-cov] Allow commas in filenames passed to `-object` flag.

Thanks, lgtm with a minor change.

Mon, Sep 14, 2:56 PM · Restricted Project
vsk added inline comments to D84988: [Coverage] Add empty line regions to SkippedRegions.
Mon, Sep 14, 11:29 AM · Restricted Project, Restricted Project, Restricted Project
vsk added a comment to D82545: [Debugify] Make the debugify aware of the original (-g) Debug Info.

@djtodoro sorry for the delay here. This looks much nicer now.

Mon, Sep 14, 10:51 AM · debug-info, Restricted Project

Wed, Sep 9

vsk added a comment to D84988: [Coverage] Add empty line regions to SkippedRegions.

Thanks for reviewing. One last thing I need to resolve is to handle the coverage test case as skipped regions are emitted for empty lines, and haven't thought a good way other than adding checks for those new skipped regions.

Wed, Sep 9, 2:04 PM · Restricted Project, Restricted Project, Restricted Project
vsk added a comment to D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage.

@alanphipps thanks for bearing with me. I think this is about ready to land. I do ask that you back out any punctuation/whitespace changes in code/tests that aren't directly modified in your diff (the clang-format-diff script can help with this). It looks like some libCoverage and test changes are no longer in the current version of this patch, as compared to the previous version (https://reviews.llvm.org/D84467?id=287269). Was that intentional?

Wed, Sep 9, 12:46 PM · Restricted Project, Restricted Project
vsk added a comment to D59715: [HotColdSplit] Reflect full cost of parameters in split penalty.

@hiraditya @rjf, thanks for sharing those numbers. At the moment I don't have the bandwidth to evaluate setting a different max-params value on x86. Let me know how you'd prefer to proceed - I'm happy to have either of you commandeer the patch, or if you prefer, I can land it in its current state.

Wed, Sep 9, 12:42 PM · Restricted Project
vsk accepted D84988: [Coverage] Add empty line regions to SkippedRegions.

I took a closer look at the lexer changes, and think that these look fine. Thanks again for working on this. LGTM with a minor change, described inline.

Wed, Sep 9, 12:35 PM · Restricted Project, Restricted Project, Restricted Project

Tue, Sep 8

vsk updated the diff for D87332: [profile] Add %t LLVM_PROFILE_FILE option to substitute $TMPDIR.
  • Document the new option.
Tue, Sep 8, 2:55 PM · Restricted Project, Restricted Project
vsk requested review of D87332: [profile] Add %t LLVM_PROFILE_FILE option to substitute $TMPDIR.
Tue, Sep 8, 2:50 PM · Restricted Project, Restricted Project
vsk added inline comments to D85670: [Instruction] Add updateLocationAfterHoist helper.
Tue, Sep 8, 1:51 PM · Restricted Project
vsk updated the diff for D85670: [Instruction] Add updateLocationAfterHoist helper.

Add a dropLocation API, and use this to implement updateLocationAfterHoist. 'dropLocation' is a more general name, and should be applicable in more contexts.

Tue, Sep 8, 1:51 PM · Restricted Project

Wed, Sep 2

vsk added a comment to D84988: [Coverage] Add empty line regions to SkippedRegions.

I'm not as familiar with the preprocessor bits, but this looks like it's in good shape.

Wed, Sep 2, 4:59 PM · Restricted Project, Restricted Project, Restricted Project
vsk added a comment to D87003: [llvm-cov] Allow commas in filenames passed to `-object` flag.

I don't think the intention was ever to require filename arguments to be comma separated (the expectation is that multiple -object arguments should be passed). This looks like a reasonable cleanup though.

Wed, Sep 2, 2:18 PM · Restricted Project

Tue, Sep 1

vsk added inline comments to D84988: [Coverage] Add empty line regions to SkippedRegions.
Tue, Sep 1, 8:14 AM · Restricted Project, Restricted Project, Restricted Project

Fri, Aug 28

vsk added a comment to D86799: [WIP] Experimental Debug Location Verifier..

Very cool. Does this fire at all in an -O0 stage2 build?

Fri, Aug 28, 11:54 AM · Restricted Project

Thu, Aug 27

vsk accepted D86000: Add an unsigned shift base sanitizer.

Lgtm, thanks.

Thu, Aug 27, 1:36 PM · Restricted Project, Restricted Project, Restricted Project
vsk added inline comments to D86000: Add an unsigned shift base sanitizer.
Thu, Aug 27, 10:41 AM · Restricted Project, Restricted Project, Restricted Project

Wed, Aug 26

vsk committed rG1f47f89a901f: [profile] Add InstrProfilingVersionVar.c.o to Darwin kext builtins (authored by vsk).
[profile] Add InstrProfilingVersionVar.c.o to Darwin kext builtins
Wed, Aug 26, 10:02 AM

Aug 18 2020

vsk accepted D86116: [Coverage] Adjust skipped regions only if {Prev,Next}TokLoc is in the same file as regions' {start, end}Loc.

Lgtm, thank you!

Aug 18 2020, 12:38 PM · Restricted Project
vsk added inline comments to D84988: [Coverage] Add empty line regions to SkippedRegions.
Aug 18 2020, 12:34 PM · Restricted Project, Restricted Project, Restricted Project
vsk added a comment to D85036: [llvm-cov] reset executation count to 0 after wrapped segment.
In D85036#2224279, @vsk wrote:
In D85036#2222873, @vsk wrote:

Hi @zequanwu, we've started seeing a regression due to this change in swift (https://ci.swift.org/view/swift-master-next/job/oss-swift-incremental-RA-osx-master-next/7818/consoleText, see the bit about 'coverage_smoke.swift'). Here's the failure we see:

           165:  165| 2| throw CustomError.Err // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
check:167'0            X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
check:167'1                                                                          with "@LINE" equal to "167"
check:167'2                                                                   ?      possible intended match
           166:  166| 2| } catch {
check:167'0     ~~~~~~~~~~~~~~~~~~
           167:  167| 1| if b { // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
check:167'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           168:  168| 1| return 1 // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}1
check:167'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           169:  169| 1| }

The issue is that line 167 ("if b {") has an execution count of 1, even though it's actually reached twice. Before this change, the wrapped count from the catch block starting on line 166 would apply on line 167, all the way through the IfStmt condition (giving an execution count of 2). But with this change, seeing as the IfStmt condition doesn't get its own region, the line execution count gets picked up from the "Then" side of the IfStmt.

I'm concerned that this nested statements generated in clang as well as swift. Do you have some idea about how to address this?

I wonder whether we can make use of gap regions to fix the clang bugs you were looking at. I noticed that this fixed an issue in instrprof-comdat.h, but it's possible that that was caused by a bug in clang that has since been fixed (per the note).

Hi, I don't know how swift generate the regions. Shouldn't IfStmt gets its own regions covering from if keyword to thenStmt? Is that intended not to do so in swift?

Thanks for taking a look. Clang and Swift both handle IfStmt in the same way, i.e. they both extend the parent region of the IfStmt through the condition. In clang, this is:

void VisitIfStmt(const IfStmt *S) {
  extendRegion(S);
  if (S->getInit())
    Visit(S->getInit());

  // Extend into the condition before we propagate through it below - this is
  // needed to handle macros that generate the "if" but not the condition.
  extendRegion(S->getCond());
  ...

I think this behavior is reasonable (it makes sense to say that the condition of an IfStmt is executed exactly as many times as the parent region containing the IfStmt).

My original thought about this is for one line if there is any new line segment which is start of region, we should ignore the wrapped line segment, and the new line segment count should be the accurate one. Do you have dumping result of segments (-debug-only=coverage-mapping) so we can take a deeper look?

Right, I thought I was on the same page as you about this. After paging a bit more of this in, I think a problem with ignoring the wrapped line segment is that it must have originated from a region that hasn't actually ended. If a line contains a new start-of-region line segment, it may well begin very late (e.g. at the end of the line), in which case it seems appropriate to consider using the count from the parent region (the wrapped count).

Here is the swift example in more detail:

func catchError2(_ b: Bool) -> Int {
  do {
    throw CustomError.Err // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
  } catch { // <<< Line 165 >>>
    if b {                // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
      return 1            // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}1
    }
  }
  return 0                // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}1
}
let _ = catchError2(true)
let _ = catchError2(false)
...
Counter in file 0 165:11 -> 169:4, #1
Counter in file 0 166:10 -> 168:6, #2
Counter in file 0 168:6 -> 169:4, (#1 - #2)
...
  163:6 -> 165:4 (count=2)
  165:11 -> 169:4 (count=2)
  166:10 -> 168:6 (count=1)
  168:6 -> 169:4 (count=1)

So, the "catch" on line 165 gets count=2. We then go to line 166 with wrapped-count=2. But we end up applying the start-of-region count on line 166, 166:10 -> 168:6 (count=1), which seems off.

Is it because it's missing the code region for condition b? In clang, the condition will have a region with count 2. In the dumping result you given, I don't see that.

Aug 18 2020, 11:43 AM · Restricted Project
vsk added a comment to D85036: [llvm-cov] reset executation count to 0 after wrapped segment.
In D85036#2222873, @vsk wrote:

Hi @zequanwu, we've started seeing a regression due to this change in swift (https://ci.swift.org/view/swift-master-next/job/oss-swift-incremental-RA-osx-master-next/7818/consoleText, see the bit about 'coverage_smoke.swift'). Here's the failure we see:

           165:  165| 2| throw CustomError.Err // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
check:167'0            X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
check:167'1                                                                          with "@LINE" equal to "167"
check:167'2                                                                   ?      possible intended match
           166:  166| 2| } catch {
check:167'0     ~~~~~~~~~~~~~~~~~~
           167:  167| 1| if b { // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}2
check:167'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           168:  168| 1| return 1 // CHECK-COV: {{ *}}[[@LINE]]|{{ *}}1
check:167'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           169:  169| 1| }

The issue is that line 167 ("if b {") has an execution count of 1, even though it's actually reached twice. Before this change, the wrapped count from the catch block starting on line 166 would apply on line 167, all the way through the IfStmt condition (giving an execution count of 2). But with this change, seeing as the IfStmt condition doesn't get its own region, the line execution count gets picked up from the "Then" side of the IfStmt.

I'm concerned that this nested statements generated in clang as well as swift. Do you have some idea about how to address this?

I wonder whether we can make use of gap regions to fix the clang bugs you were looking at. I noticed that this fixed an issue in instrprof-comdat.h, but it's possible that that was caused by a bug in clang that has since been fixed (per the note).

Hi, I don't know how swift generate the regions. Shouldn't IfStmt gets its own regions covering from if keyword to thenStmt? Is that intended not to do so in swift?

Aug 18 2020, 10:32 AM · Restricted Project

Aug 17 2020

vsk added a comment to D85036: [llvm-cov] reset executation count to 0 after wrapped segment.

Hi @zequanwu, we've started seeing a regression due to this change in swift (https://ci.swift.org/view/swift-master-next/job/oss-swift-incremental-RA-osx-master-next/7818/consoleText, see the bit about 'coverage_smoke.swift'). Here's the failure we see:

Aug 17 2020, 5:11 PM · Restricted Project
vsk added inline comments to D84988: [Coverage] Add empty line regions to SkippedRegions.
Aug 17 2020, 10:30 AM · Restricted Project, Restricted Project, Restricted Project
vsk added a comment to D84988: [Coverage] Add empty line regions to SkippedRegions.

Hi @zequanwu, are you looking for review for this patch? I wasn't sure because of the WIP label.

Aug 17 2020, 10:03 AM · Restricted Project, Restricted Project, Restricted Project

Aug 14 2020

vsk added a comment to D86000: Add an unsigned shift base sanitizer.
In D86000#2219322, @jfb wrote:
In D86000#2219288, @vsk wrote:

It'd be nice to fold the new check into an existing sanitizer group to bring this to a wider audience. Do you foresee adoption issues for existing -fsanitize=integer adopters? Fwiw some recently-added implicit conversion checks were folded in without much/any pushback.

integer does "not actually UB checks", right? I can certainly put it in there if you think I won't get yelled at 😄

Aug 14 2020, 4:36 PM · Restricted Project, Restricted Project, Restricted Project
vsk added inline comments to D86000: Add an unsigned shift base sanitizer.
Aug 14 2020, 4:35 PM · Restricted Project, Restricted Project, Restricted Project
vsk added inline comments to D84467: Add support for Branch Coverage in LLVM Source-Based Code Coverage.
Aug 14 2020, 4:21 PM · Restricted Project, Restricted Project
vsk added a comment to D86000: Add an unsigned shift base sanitizer.

It'd be nice to fold the new check into an existing sanitizer group to bring this to a wider audience. Do you foresee adoption issues for existing -fsanitize=integer adopters? Fwiw some recently-added implicit conversion checks were folded in without much/any pushback.

Aug 14 2020, 3:42 PM · Restricted Project, Restricted Project, Restricted Project
vsk added a comment to D85628: [HotColdSplitting] Add command line options for supplying cold function names via user input..

I have two questions for this patch:

  • Why do we need another way to explicitly tell compiler that some functions are cold, rather than using existing mechanisms?
    • If this is for a random cold function from user code, then using profile or in-source annotation should be the way to go, which is more scalable and more sustainable.
    • If this is for special (generated) functions, e.g. __cxa_guard_acquire, then we shouldn't even need to tell the compiler. It's like we don't have to tell compiler EH pad, noreturn functions are cold, instead compiler should figure this out by itself. _cxa_guard_acquire is a generated function with very specific semantic, so I think it's similar to noreturn, EH pads in that regard.
  • If we have to tell compiler extra hotness/coldness info, why do we do it just HCS? hotness is very general, and could benefit many opts. Introducing a channel to tell specific optimization about hotness is not good design in general (imagine 10 passes each has its own way of getting hotness through a bunch of switches). If we really have to do this, I'd say we should do it in the framework, e.g. the static analysis part of BFI.
Aug 14 2020, 1:28 PM · Restricted Project

Aug 12 2020

vsk accepted D85176: [Coverage] Enable emitting gap area between macros.

Thanks, looks good to me! Please check for any issues in a stage2 coverage build before landing this.

Aug 12 2020, 2:18 PM · Restricted Project
vsk committed rGd49aedd315e3: Build a flat LLDB.framework for embedded Darwin targets (authored by vsk).
Build a flat LLDB.framework for embedded Darwin targets
Aug 12 2020, 1:35 PM
vsk closed D85770: Build a flat LLDB.framework for embedded Darwin targets.
Aug 12 2020, 1:35 PM · Restricted Project
vsk added a comment to D85628: [HotColdSplitting] Add command line options for supplying cold function names via user input..

I’m not convinced this is a good idea. In what use case is it not possible to mark up relevant functions? It doesn’t make sense to me to make alternations to standard library functions within the compiler. It seems better to simply patch the standard library. In some cases llvm does infer function attributes for library functions, but these are generally lower level attributes that can’t be specified at the source level, and the attribute is made available to other passes in the pipeline.

Aug 12 2020, 1:16 PM · Restricted Project

Aug 11 2020

vsk added a comment to D85628: [HotColdSplitting] Add command line options for supplying cold function names via user input..

Any reason not to mark up the relevant functions with __attribute__((cold)) directly?

Aug 11 2020, 6:05 PM · Restricted Project
vsk added inline comments to D85555: [InstCombine] Remove dbg.values describing contents of dead allocas.
Aug 11 2020, 5:57 PM · Restricted Project
vsk added inline comments to D59715: [HotColdSplit] Reflect full cost of parameters in split penalty.
Aug 11 2020, 5:36 PM · Restricted Project
vsk updated the diff for D85670: [Instruction] Add updateLocationAfterHoist helper.

Preserve the inlinedAt location unless the parent function has a scope.

Aug 11 2020, 3:39 PM · Restricted Project
vsk updated the summary of D85670: [Instruction] Add updateLocationAfterHoist helper.
Aug 11 2020, 3:38 PM · Restricted Project
vsk added a comment to D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages..
In D85705#2211073, @vsk wrote:

This looks very cool, thanks @clayborg! I think using JSON to describe the trace data (what kind of trace is this, what's in it, etc.) sounds reasonable.

For "trace load", I get the plugin for the JSON file by matching it up with the "name" field in the JSON, but I don't store the "trace_sp" anywhere. We will need to store it with the target that we create, or for later commands add it to a target that is stopped when the trace data is loaded via the process interface (through lldb-server is the current thinking for this).

Have you considered what might happen if there are multiple targets covered by a single trace? Strawman proposal: would it make sense to register the trace with a Debugger instance? This can be a list of traces if it makes sense to support debugging more than one trace at a time.

I hadn't thought of that but that does make sense!

We can work this all into the JSON format. We should actually make a schema for the common parts of information that should be represented in the JSON and also allow each plug-in to supply a schema for the parts that is requires in the JSON.

Some ideas that this information should contain:

  • array of process infos dictionaries
  • process info dictionary
    • pid
    • array of shared library dictionaries
  • shared library dictionary:
    • original path
    • UUID if available
    • MD5 of file if no UUID
    • load location
    • optional URL to download

And LLDB will easily be able to load up N targets with everything setup correctly.

Aug 11 2020, 3:35 PM · Restricted Project, Restricted Project
vsk reopened D85670: [Instruction] Add updateLocationAfterHoist helper.
Aug 11 2020, 2:59 PM · Restricted Project
vsk added a reverting change for rG4a646ca9e2ca: [Instruction] Add updateLocationAfterHoist helper: rG30c1633386e7: Revert "[Instruction] Add updateLocationAfterHoist helper".
Aug 11 2020, 2:54 PM
vsk committed rG30c1633386e7: Revert "[Instruction] Add updateLocationAfterHoist helper" (authored by vsk).
Revert "[Instruction] Add updateLocationAfterHoist helper"
Aug 11 2020, 2:54 PM
vsk added a reverting change for D85670: [Instruction] Add updateLocationAfterHoist helper: rG30c1633386e7: Revert "[Instruction] Add updateLocationAfterHoist helper".
Aug 11 2020, 2:54 PM · Restricted Project
vsk added inline comments to D85770: Build a flat LLDB.framework for embedded Darwin targets.
Aug 11 2020, 2:07 PM · Restricted Project
vsk committed rG4a646ca9e2ca: [Instruction] Add updateLocationAfterHoist helper (authored by vsk).
[Instruction] Add updateLocationAfterHoist helper
Aug 11 2020, 2:05 PM
vsk closed D85670: [Instruction] Add updateLocationAfterHoist helper.
Aug 11 2020, 2:05 PM · Restricted Project
vsk requested review of D85770: Build a flat LLDB.framework for embedded Darwin targets.
Aug 11 2020, 12:22 PM · Restricted Project
vsk added a comment to D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages..

This looks very cool, thanks @clayborg! I think using JSON to describe the trace data (what kind of trace is this, what's in it, etc.) sounds reasonable.

Aug 11 2020, 11:51 AM · Restricted Project, Restricted Project
vsk added inline comments to D85555: [InstCombine] Remove dbg.values describing contents of dead allocas.
Aug 11 2020, 11:36 AM · Restricted Project

Aug 10 2020

vsk added a comment to D83047: [LiveDebugValues] 2/4 Add instruction-referencing LiveDebugValues implementation.

Hi @jmorse, thanks for the updates on the DbgValue::{Proposed,NoVal} scheme.

Aug 10 2020, 1:54 PM · debug-info, Restricted Project
vsk updated the diff for D59715: [HotColdSplit] Reflect full cost of parameters in split penalty.

Rebase.

Aug 10 2020, 1:01 PM · Restricted Project
vsk added inline comments to D59715: [HotColdSplit] Reflect full cost of parameters in split penalty.
Aug 10 2020, 1:01 PM · Restricted Project
vsk accepted D85433: [compiler-rt][ubsan][test] XFAIL TypeCheck/misaligned.cpp on Sparc.

Thanks!

Aug 10 2020, 12:27 PM · Restricted Project
vsk updated the diff for D85670: [Instruction] Add updateLocationAfterHoist helper.

Add doxygen comment.

Aug 10 2020, 10:48 AM · Restricted Project
vsk requested review of D85670: [Instruction] Add updateLocationAfterHoist helper.
Aug 10 2020, 10:46 AM · Restricted Project
vsk updated the diff for D85555: [InstCombine] Remove dbg.values describing contents of dead allocas.

Fix two check lines to use the right check prefix.

Aug 10 2020, 9:43 AM · Restricted Project
vsk added inline comments to D85555: [InstCombine] Remove dbg.values describing contents of dead allocas.
Aug 10 2020, 9:42 AM · Restricted Project

Aug 7 2020

vsk requested review of D85561: [BasicBlockUtils] Factor MergeBlockIntoPredecessor args into a struct, NFC.
Aug 7 2020, 2:58 PM · Restricted Project
vsk updated subscribers of D85555: [InstCombine] Remove dbg.values describing contents of dead allocas.
Aug 7 2020, 2:16 PM · Restricted Project
vsk requested review of D85555: [InstCombine] Remove dbg.values describing contents of dead allocas.
Aug 7 2020, 2:15 PM · Restricted Project
vsk added inline comments to D85433: [compiler-rt][ubsan][test] XFAIL TypeCheck/misaligned.cpp on Sparc.
Aug 7 2020, 11:57 AM · Restricted Project
vsk added inline comments to D85433: [compiler-rt][ubsan][test] XFAIL TypeCheck/misaligned.cpp on Sparc.
Aug 7 2020, 11:29 AM · Restricted Project

Aug 6 2020

vsk added a comment to D60913: [GVN+LICM] Use line 0 locations for better crash attribution.

Sounds good, I've put this on my queue and hope to take a stab at it by Monday (8/10).

Aug 6 2020, 4:27 PM · Restricted Project
vsk added a comment to D59715: [HotColdSplit] Reflect full cost of parameters in split penalty.

I haven't forgotten about this! Hope to have an update by Monday (8/10).

Aug 6 2020, 4:22 PM · Restricted Project
vsk added a comment to D60913: [GVN+LICM] Use line 0 locations for better crash attribution.
In D60913#2194847, @vsk wrote:

Using zero on non-call locations might bloat the line table a fair bit. May be better to let those locations get flow-on from whatever other instructions are around?

At least that, I think, is the current policy of "merge debug locations", is it not?

The current policy recommends using a merged debug location here (ref),

I'm not sure it does - since this code isn't doing any merging - it's hoisting a single instruction, which I don't think is covered by that part of the documentation, is it?

Aug 6 2020, 4:09 PM · Restricted Project
vsk accepted D85243: Factor out common code from the iPhone/AppleTV/WatchOS simulator platform plugins. (NFC).

Looks good to me.

Aug 6 2020, 3:39 PM · Restricted Project
vsk added a comment to D85433: [compiler-rt][ubsan][test] XFAIL TypeCheck/misaligned.cpp on Sparc.

As an alternative, would it be workable to convert this test to use -fsanitize-trap? That way, the test program would trap after issuing a runtime error (it can be run with not --crash %run ...).

Aug 6 2020, 1:27 PM · Restricted Project
vsk added a comment to D85243: Factor out common code from the iPhone/AppleTV/WatchOS simulator platform plugins. (NFC).

I gave this another look-over, and while I didn't spot anything troubling, I'm not terribly familiar with this code. I'll give this a third pass later today if review is still needed.

Aug 6 2020, 1:23 PM · Restricted Project

Aug 5 2020

vsk added inline comments to D79279: Add overloaded versions of builtin mem* functions.
Aug 5 2020, 10:46 AM · Restricted Project, Restricted Project

Aug 4 2020

vsk added inline comments to D85176: [Coverage] Enable emitting gap area between macros.
Aug 4 2020, 4:51 PM · Restricted Project
vsk added a comment to D85036: [llvm-cov] reset executation count to 0 after wrapped segment.

@zequanwu it looks like the llvm-cov tests need to be updated to match this change.

Aug 4 2020, 4:44 PM · Restricted Project
vsk accepted D85036: [llvm-cov] reset executation count to 0 after wrapped segment.

LGTM, thanks. The count from the wrapped segment shouldn't be preferred over the count from a region-entry segment that begins on some line.

Aug 4 2020, 4:42 PM · Restricted Project
vsk added a comment to D60913: [GVN+LICM] Use line 0 locations for better crash attribution.

Using zero on non-call locations might bloat the line table a fair bit. May be better to let those locations get flow-on from whatever other instructions are around?

At least that, I think, is the current policy of "merge debug locations", is it not?

Aug 4 2020, 4:14 PM · Restricted Project
vsk added inline comments to D85243: Factor out common code from the iPhone/AppleTV/WatchOS simulator platform plugins. (NFC).
Aug 4 2020, 3:31 PM · Restricted Project
vsk added a comment to D85155: [UBSan] Increase robustness of tests.

Thanks for fixing this!

Aug 4 2020, 8:21 AM · Restricted Project

Jul 30 2020

vsk committed rG896f797b8bb7: [profile] Remove dependence on getpagesize from InstrProfilingBuffer.c.o (authored by vsk).
[profile] Remove dependence on getpagesize from InstrProfilingBuffer.c.o
Jul 30 2020, 4:23 PM
vsk added a reviewer for D84891: WIP: [Verifier] Flag dbg.declares which specify different addresses for the same fragment: davide.

The SourceLevelDebugging documentation update makes sense to me. And this check should probably make use of the DebugVariable wrapper.

Jul 30 2020, 10:46 AM · Restricted Project
vsk added a comment to D84771: [llvm-profdata] Add intersect/exclude subcommands to aid differential code coverage analysis..

I haven't had a chance to look at this patch yet, but have no objection to adding these subcommands.

Jul 30 2020, 10:44 AM · Restricted Project

Jul 29 2020

vsk added reviewers for D59715: [HotColdSplit] Reflect full cost of parameters in split penalty: rjf, hiraditya.
Jul 29 2020, 5:40 PM · Restricted Project
vsk added a comment to D84468: [HotColdSplitting] Add SplittingDelta option to enable splitting more small blocks.

@rjf thanks for sharing those numbers!

Jul 29 2020, 5:40 PM · Restricted Project
vsk committed rG618a0c0d3bd3: [profile] Add InstrProfilingInternal.c.o to Darwin kext builtins (authored by vsk).
[profile] Add InstrProfilingInternal.c.o to Darwin kext builtins
Jul 29 2020, 5:24 PM
vsk updated the summary of D84891: WIP: [Verifier] Flag dbg.declares which specify different addresses for the same fragment.
Jul 29 2020, 3:58 PM · Restricted Project
vsk added a comment to D84891: WIP: [Verifier] Flag dbg.declares which specify different addresses for the same fragment.

@modocache Do you know anyone who could take a look at coro-debug-frame-variable.ll? This is what I'm seeing:

Jul 29 2020, 3:58 PM · Restricted Project
vsk updated the diff for D84891: WIP: [Verifier] Flag dbg.declares which specify different addresses for the same fragment.

Ignore inlined entities. It's not unexpected for a function that's inlined multiple times to introduce dbg.declares for the same inlined entity.

Jul 29 2020, 3:56 PM · Restricted Project
vsk requested review of D84891: WIP: [Verifier] Flag dbg.declares which specify different addresses for the same fragment.
Jul 29 2020, 2:49 PM · Restricted Project