Page MenuHomePhabricator

jmorse (Jeremy Morse)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 19 2018, 2:57 AM (262 w, 5 d)

Recent Activity

Yesterday

jmorse accepted D147312: [Assignment Tracking][SROA] Handle DIArgList in migrateDebugInfo.

LGTM; could you expand the comment where you set the kill location a little more, to explicitly explain why we can't use the DIArgList. As I understand it, it's because we'd have to compute the value, then start fragmenting it up, which is a large amount of effort for a rare scenario.

Fri, Mar 31, 4:05 AM · Restricted Project, Restricted Project, debug-info

Thu, Mar 30

jmorse accepted D147060: [MachineSink] Fix an assertion failure in isCycleInvariant.

LGTM -- when landing could you add a short comment to the test indicating which function used to crash, that'll ease the life of anyone reading the test in the future and let them know exactly what's being tested.

Thu, Mar 30, 7:37 AM · Restricted Project, Restricted Project
jmorse updated the diff for D145516: [Inliner] Avoid excessive inlining through devirtualised calls.

I've changed my mind: actually there are scenarios where being inlined into a different context should enable inlining that was previously skipped, for example where an unlikely branch can statically found to be always true after inlining, justifying inlining whatever's down that branch. This has exposed the deeper issue and I've now got an updated test that does not rely on profile data.

Thu, Mar 30, 7:03 AM · Restricted Project, Restricted Project

Wed, Mar 29

jmorse accepted D146987: [Assignment Tracking] Enable by default.

LGTM -- this is after all just a switch-throw to enable everything that's in tree already (which has all been reviewed). The expected compile-time regression on ReleaseLTO-g is roughly 1.2% right, in exchange for greater variable location coverage?

Wed, Mar 29, 8:20 AM · Restricted Project, Restricted Project, debug-info
jmorse added a comment to D145516: [Inliner] Avoid excessive inlining through devirtualised calls.

(Oh looks, Phab posts when you hit control+enter; I've edited more data into the earlier comment)

Wed, Mar 29, 5:32 AM · Restricted Project, Restricted Project
jmorse added a comment to D145516: [Inliner] Avoid excessive inlining through devirtualised calls.

Interesting -- I've dug into the cost modelling, and I think you're correct that there's an issue, but it's even less enjoyable. It looks like the effect of the branch-weights successfully suppresses longname being inlined into rec_call_to_longname because it's a cold callsite, in the first instance. However, once rec_call_to_longname is inlined into a, the same computations round all the frequencies to zero, and inlining starts to happen repeatedly through both rec_call_to_longname and longname, giving the exponential behaviour. Without the branch weights, inlining longname into rec_call_to_longname creates a directly recursive function that never gets inlined again.

Wed, Mar 29, 5:30 AM · Restricted Project, Restricted Project
jmorse accepted D146980: [Assignment Tracking] Coalesce dbg loc definitions with contiguous fragments.

LGTM

Wed, Mar 29, 3:23 AM · Restricted Project, Restricted Project, debug-info
jmorse added a comment to D147060: [MachineSink] Fix an assertion failure in isCycleInvariant.

A more generalised solution I believe would be to skip debug instructions while examining the basic block -- that would mean

  • Using an iterator-based for loop rather than a range based loop, and
  • skipDebugInstructionsForward to increment past any debug instructions at the start of each iteration
Wed, Mar 29, 1:20 AM · Restricted Project, Restricted Project

Tue, Mar 28

jmorse added a comment to D146978: [Assignment Tracking] Improve removeRedundantDbgLocsUsingBackwardScan.

I don't think there's been a change - I would argue that has always been the case.

Tue, Mar 28, 8:41 AM · Restricted Project, Restricted Project, debug-info
jmorse added a comment to D146893: [ADT] Work around MSVC bug affecting `get(enumerator_result)`.

Note that I'm seeing this too with Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29915 for x64.

Tue, Mar 28, 8:35 AM · Restricted Project, Restricted Project
jmorse accepted D146615: [Assignment Tracking] Give -fexperimental-assignment-tracking flag 3 options.

I agree with @jryans analysis, otherwise LGTM, you might want to wait a bit for input from @probinson who enjoys looking at clang-switch tests.

Tue, Mar 28, 7:56 AM · Restricted Project, Restricted Project, debug-info
jmorse added a comment to D146980: [Assignment Tracking] Coalesce dbg loc definitions with contiguous fragments.

Good saving; in the future we might find different ways of representing variable fragments and communicating them to LiveDebugValues, or make LDV smarter, but it's good to get this benefit now. I think the more structured fragment model @scott.linder proposed would deliver the sort of guarantees we'd need.

Tue, Mar 28, 7:12 AM · Restricted Project, Restricted Project, debug-info
jmorse accepted D146978: [Assignment Tracking] Improve removeRedundantDbgLocsUsingBackwardScan.

LGTM with one question -- there's a semantic change whereby a debug intrinsic with no fragment is considered as covering the whole variable, right? (I think this is uncontroversial).

Tue, Mar 28, 3:59 AM · Restricted Project, Restricted Project, debug-info
jmorse accepted D146985: [Assignment Tracking][NFC] Cache debug-info-assignment-tracking module flag.

LGTM

Tue, Mar 28, 3:35 AM · Restricted Project, Restricted Project, debug-info
jmorse accepted D145515: [Assignment Tracking][NFC] Only calculate fragment overlaps for partially stack homed variables.

LGTM

Tue, Mar 28, 3:34 AM · Restricted Project, Restricted Project

Thu, Mar 23

jmorse accepted D144732: [Assignment Tracking] Elide a map copy in some cases.

LGTM with a question; this does introduce knarlyness, so we should revisit this if we manage to make the pass faster.

Thu, Mar 23, 3:00 AM · Restricted Project, Restricted Project

Wed, Mar 15

jmorse added a comment to D145515: [Assignment Tracking][NFC] Only calculate fragment overlaps for partially stack homed variables.

/me squints -- so if no part of the variable ends up on the stack, this is effectively early-exiting this portion of analysis?

Wed, Mar 15, 10:04 AM · Restricted Project, Restricted Project

Mon, Mar 13

jmorse accepted D145943: [AArchExpandPseudo] Preserve instruction debug number in expansions.

LGTM, assuming that the instructions affected always have the same signature / format. (I know very little aarch64).

Mon, Mar 13, 8:19 AM · Restricted Project, Restricted Project
jmorse added a comment to D140900: [DebugInfo] Print empty MDTuples wrapped in MetadataAsValue inline.

I agree with Stephen, it's good but flipping the FromValue flag doesn't seem right.

Mon, Mar 13, 5:01 AM · Restricted Project, Restricted Project, debug-info
jmorse accepted D144652: [Assignment Tracking] getIntrinsicInstrCost: set dbg.assign cost to zero.

LGTM with inline rider

Mon, Mar 13, 4:42 AM · Restricted Project, Restricted Project, debug-info
jmorse added a comment to D144652: [Assignment Tracking] getIntrinsicInstrCost: set dbg.assign cost to zero.

This looks good, but in the test outputs shouldn't the cost be zero? Auto-regenerated without rebuilding opt perhaps? (See inline comment)

Mon, Mar 13, 3:55 AM · Restricted Project, Restricted Project, debug-info
jmorse added a comment to D144732: [Assignment Tracking] Elide a map copy in some cases.

Could you add the context for review; also, are there any clear winners in CTMark that justify this change? 0.05% is close to noise, and the function complexity increases substantially.

Mon, Mar 13, 3:52 AM · Restricted Project, Restricted Project

Fri, Mar 10

jmorse accepted D145759: [MachineCombiner] Preserve debug instruction number.

LGTM; you might consider making it a MIR test that just runs the machine-combiner pass, but I don't think that'll have any material advantage over an .ll test, the only difference is making the exact input to machine-combiner explicitly visible.

Fri, Mar 10, 2:11 AM · Restricted Project, Restricted Project

Wed, Mar 8

jmorse requested review of D145565: [DebugInfo][InstrRef] Instrument x86 CMOV conversion to preserve variable values.
Wed, Mar 8, 3:01 AM · Restricted Project, Restricted Project

Tue, Mar 7

jmorse added a comment to D145168: Move DBG_VALUE's that depend on loads to after a load if the load is moved due to the pre register allocation ld/st optimization pass.

This is a good improvement, many thanks for working in this area. The overall approach is good, I've got a couple of high level comments:

  • I believe you'll need to erase elements from LocalVarMap once a DBG_VALUE is sunk -- if a later, unrelated DBG_VALUE for the same variable is encountered in the same block, then there's no need terminate the earlier sunk DBG_VALUE,
  • You should use the DebugVariable class to identify variables, as that combines inlining and fragment information into a single object. Using just DILocalVariable risks duplicate instances of the same variable from different inlining contexts being merged.
Tue, Mar 7, 10:28 AM · debug-info, Restricted Project, Restricted Project
jmorse requested review of D145516: [Inliner] Avoid excessive inlining through devirtualised calls.
Tue, Mar 7, 8:51 AM · Restricted Project, Restricted Project

Feb 10 2023

jmorse accepted D143716: [llvm-debuginfo-analyzer] LLVM 16.0.0-rc1 Failing test on osx-64..

If I understand correctly, this is moving the tool-global string pool from a header-function into something with one-definition -- definitely the right direction to take. I think Orlando might have said that a context-object-with-string-pool is probably the best overall solution, but this is a good improvement for now.

Feb 10 2023, 3:31 AM · Restricted Project, Restricted Project, debug-info

Feb 7 2023

jmorse committed rG2399497c9d68: [debug-info] Followup to e0374fb2f4, avoid changing codegen (authored by jmorse).
[debug-info] Followup to e0374fb2f4, avoid changing codegen
Feb 7 2023, 4:27 AM · Restricted Project, Restricted Project
jmorse added a comment to D140404: Patch for dbg variable instrinsics to point towards cloned values in JumpThreading.

Cool, sounds like this is just a case of mistaken-function-identity, thanks for catching this @uabelho. I'll drop in a follow-up patch shortly to use GetValueAtEndOfBlock

Feb 7 2023, 3:49 AM · Restricted Project, Restricted Project
jmorse added a comment to D140404: Patch for dbg variable instrinsics to point towards cloned values in JumpThreading.

/me squints -- I think the idea here was that if SSAUpdater has a value at the end of the block we can use that and it won't cause new codegen; but then we're using GetValueInMiddleOfBlock, which will generate code in certain circumstances.

Feb 7 2023, 3:30 AM · Restricted Project, Restricted Project
jmorse added a comment to D142019: ARM: skip debug instructions when matching jump-table patterns..

LGTM from a debug-info perspective; I know next to nothing about ARM though so can't venture an opinion there. A smaller test would be appreciated if that's possible.

Feb 7 2023, 2:54 AM · Restricted Project, Restricted Project

Jan 31 2023

jmorse accepted D142882: [Assignment Tracking][SROA] Delete dbg.assigns linked to rewritten stores.

LGTM -- and for my understanding, this is because the assignments have been replaced, leaving the dbg.assign hanging around in the case of DSE would be the correct behaviour, yes?

Jan 31 2023, 3:50 AM · Restricted Project, Restricted Project, debug-info

Jan 30 2023

jmorse accepted D142654: [LiveDebugValues] Allow EntryValue with OP_deref expressions.

LGTM3

Jan 30 2023, 1:16 AM · Restricted Project, Restricted Project
jmorse added a comment to D142787: [GVN] Don't count debug instructions when limit the number of checked instructions.

From a debug-info perspective this LGTM, with the test improvement to reduce the size -- I'm not familiar with all of GVN though. The objective of ensuring that debug-info can't affect the code generated is an important one.

Jan 30 2023, 1:14 AM · debug-info, Restricted Project, Restricted Project

Jan 27 2023

jmorse added a comment to D142556: [DebugInfo] Merge partially matching chains of inlined locations.

I've run out of time to read the code this week, but it sounds like a good approach to me, I'll give my debugger colleagues a prod about it too.

Jan 27 2023, 10:34 AM · Restricted Project, Restricted Project, debug-info
jmorse added a comment to D142654: [LiveDebugValues] Allow EntryValue with OP_deref expressions.

LGTM, but I'll leave to @djtodoro to approve. Support in the other flavour of LiveDebugValues would indeed be appreciated.

Jan 27 2023, 8:52 AM · Restricted Project, Restricted Project

Jan 23 2023

jmorse added inline comments to D142019: ARM: skip debug instructions when matching jump-table patterns..
Jan 23 2023, 4:33 AM · Restricted Project, Restricted Project
jmorse committed rGb2cf024280b3: [DebugInfo][CSInfo] Don't use clobbered registers as locations (authored by jmorse).
[DebugInfo][CSInfo] Don't use clobbered registers as locations
Jan 23 2023, 4:21 AM · Restricted Project, Restricted Project
jmorse closed D141279: [DebugInfo][CSInfo] Avoid using clobbered registers as call-site argument locations.
Jan 23 2023, 4:21 AM · Restricted Project, Restricted Project
jmorse added a comment to D138757: [zero-call-used-regs] Mark only non-debug instruction's register as used.

I am not able to complete 2nd and 4th part. Any idea what may be wrong.

[...]

error: YAML:99:1: unknown key 'debugInstrRef'
debugInstrRef: true
^~~~~~~~~~~~~

[...]

subprocess.CalledProcessError: Command 'llc -run-pass=prologepilog -o - -x mir' returned non-zero exit status 1.

Jan 23 2023, 2:28 AM · Restricted Project, Restricted Project

Jan 20 2023

jmorse committed rG9f8544713ad8: [DebugInfo] Store instr-ref mode of MachineFunction in member (authored by jmorse).
[DebugInfo] Store instr-ref mode of MachineFunction in member
Jan 20 2023, 6:47 AM · Restricted Project, Restricted Project
jmorse closed D141387: [DebugInfo] Store instr-ref/DBG_VALUE mode of a MachineFunction in a member variable.
Jan 20 2023, 6:47 AM · Restricted Project, Restricted Project

Jan 18 2023

jmorse updated the diff for D141387: [DebugInfo] Store instr-ref/DBG_VALUE mode of a MachineFunction in a member variable.

Apply review comments; I've also had an attack of conscience, and added some more check lines:

  • llvm/test/DebugInfo/x86/instr-ref-flag.ll -- check that the MIR function key gets produced with true or false values, according to the command line flags,
  • llvm/test/DebugInfo/MIR/InstrRef/instr-ref-roundtrip.mir -- check that the flag makes its way from the input to the output.
Jan 18 2023, 7:33 AM · Restricted Project, Restricted Project
jmorse updated the diff for D141279: [DebugInfo][CSInfo] Avoid using clobbered registers as call-site argument locations.

Use a type alias instead of SmallSet directly,

Jan 18 2023, 6:50 AM · Restricted Project, Restricted Project
jmorse abandoned D69178: [DebugInfo] Use DBG_VALUEs IsIndirect field for describing stack spills.

(Clearing queue of old revisions) -- things have moved on a lot since this was written, I think we're close to just killing off the IsIndirect field completely and putting everything into DIExpressions.

Jan 18 2023, 6:28 AM · Restricted Project, Restricted Project
jmorse abandoned D70676: [DebugInfo] Don't repeatedly created undef DBG_VALUEs during machine-sinking.

(Clearing out old revisions) -- it's been so long that this is unlikely to land. This was also written at the point where I was realising that to handle DBG_VALUE movement in machinesink, we basically need to re-compute SSA form each time.

Jan 18 2023, 6:27 AM · Restricted Project, Restricted Project

Jan 17 2023

jmorse added a comment to D141381: [codegen] Store address of indirect arguments on the stack.

While SROA will not touch this:

define @foo(ptr %arg) {
   call void @llvm.dbg.declare(%arg, [...], metadata !DIExpression())

It completely destroys the debug information provided by:

define @foo(ptr %arg) {
   %ptr_storage = alloca ptr
   store ptr %arg, ptr %ptr_storage
   call void @llvm.dbg.declare(%ptr_storage, [...], metadata !DIExpression(DW_OP_deref))
Jan 17 2023, 3:35 AM · Restricted Project, Restricted Project

Jan 16 2023

jmorse updated subscribers of D140089: [MemCpyOpt] Add a stack-move optimization to opportunistically merge allocas together..

Drive-by comment after spotting this question:

Jan 16 2023, 8:44 AM · Restricted Project, Restricted Project

Jan 10 2023

jmorse retitled D141387: [DebugInfo] Store instr-ref/DBG_VALUE mode of a MachineFunction in a member variable from [DebugInfo] Make instr-ref/DBG_VALUE mode of a MachineFunction to [DebugInfo] Store instr-ref/DBG_VALUE mode of a MachineFunction in a member variable.
Jan 10 2023, 7:26 AM · Restricted Project, Restricted Project
jmorse requested review of D141387: [DebugInfo] Store instr-ref/DBG_VALUE mode of a MachineFunction in a member variable.
Jan 10 2023, 7:24 AM · Restricted Project, Restricted Project
jmorse accepted D140373: [WebAssembly][LiveDebugValues] Handle target index defs.

LGTM

Jan 10 2023, 1:55 AM · Restricted Project, Restricted Project
jmorse accepted D138943: [WebAssembly] Use LiveDebugValues analysis.

This all LGTM; if you're happy with the comments for struct WasmLoc then that's fine too.

Jan 10 2023, 1:47 AM · Restricted Project, Restricted Project

Jan 9 2023

jmorse requested review of D141279: [DebugInfo][CSInfo] Avoid using clobbered registers as call-site argument locations.
Jan 9 2023, 5:58 AM · Restricted Project, Restricted Project

Jan 6 2023

jmorse added a comment to D141131: [AssignmentTracking][Docs] Add new metadata to LangRef.rst and link to AssignmentTracking.md.

Rather than store instructions, "instructions that store", seeing how you're marking some memory intrinsics with DIAssignID too?

Jan 6 2023, 6:17 AM · Restricted Project, Restricted Project, debug-info
jmorse added a comment to D133927: [DebugInfo] Add support for variadic DBG_INSTR_REFs in LiveDebugValues.

Yes - after all the input debug value instructions are converted to the internal representation (DbgValue) in the instruction referencing live debug values, variadic DBG_INSTR_REFs follow the same code paths as DBG_VALUE_LISTs, which had test coverage added when they were introduced. Notably those patches added unit tests, and this patch has nothing to add on top of them. If you think test coverage could be expanded though then I should be able to generate some more cases, it might not be a bad idea to cast a wide net in case there are any rare or insidious errors.

Jan 6 2023, 2:25 AM · Restricted Project, Restricted Project, debug-info

Dec 27 2022

jmorse added inline comments to D133311: [Assignment Tracking][16/*] Account for assignment tracking in mldst-motion.
Dec 27 2022, 6:20 AM · Restricted Project, Restricted Project, debug-info

Dec 24 2022

jmorse added a comment to D140373: [WebAssembly][LiveDebugValues] Handle target index defs.

Thanks for assurance about the stack-popping being addressed elsewhere -- it might be worth putting a comment in D138943 next to the definition of struct WasmLoc about that being a deliberate non-objective, to enlighten future readers.

Dec 24 2022, 6:12 AM · Restricted Project, Restricted Project
jmorse updated subscribers of D133925: [DebugInfo] Fix: Variables that have no non-empty values being emitted when they have a DBG_VALUE_LIST.

Thanks @vitalybuka , looks like we'll get back to this after the holidays.

Dec 24 2022, 4:55 AM · Restricted Project, Restricted Project, debug-info

Dec 21 2022

jmorse committed rG2b1d45b227bd: [NFC] Add --check-globals to an autogen test cmdline (authored by jmorse).
[NFC] Add --check-globals to an autogen test cmdline
Dec 21 2022, 8:49 AM · Restricted Project, Restricted Project

Dec 20 2022

jmorse accepted D140412: [DebugInfo] Unify location selection logic for values in InstrRefBasedImpl.

Slightly twitchy about there being a zero LocIdx -- I think I tried to suppress there being any default-constructed LocIdxes out there. On the other hand, it comes with a sentinel value attached, so it's no big deal.

Dec 20 2022, 9:26 AM · Restricted Project, Restricted Project, debug-info
jmorse added a comment to D138943: [WebAssembly] Use LiveDebugValues analysis.

I removed all register-based (= dangling) DBG_VALUEs for Wasm in D139675; WebAssemblyDebugFixup runs right before this analysis. So now we don't have any virtual registers participating here, and I re-enabled the physical register assertion, so we can maintain LocIndex only deals with physical registers.

Dec 20 2022, 8:30 AM · Restricted Project, Restricted Project
jmorse accepted D140006: fix jump threading failing to update cloned dbg.values.

LGTM

Dec 20 2022, 7:20 AM · Restricted Project, Restricted Project
jmorse accepted D133926: [DebugInfo] Allow non-stack_value variadic expressions and use in DBG_INSTR_REF.

Looks good (leaning largely on my prior review).

Dec 20 2022, 7:18 AM · Restricted Project, Restricted Project, debug-info
jmorse accepted D133927: [DebugInfo] Add support for variadic DBG_INSTR_REFs in LiveDebugValues.

Looks good, but with what feels like low test coverage: remind me, the plumbing required to track variadic locations in LiveDebugValues already landed, and we have tests to cover those new code paths, yes?

Dec 20 2022, 6:59 AM · Restricted Project, Restricted Project, debug-info

Dec 14 2022

jmorse requested changes to D140006: fix jump threading failing to update cloned dbg.values.

You can iterate over the values using DbgVariableIntrinsic::location_ops() and you can replace them using DbgVariableIntrinsic::replaceVariableLocationOp.

It may be acceptable to leave that as a "TODO" item - I'll leave that up to @jmorse. If it's decided to leave the patch in its current form then please take a look at the inline comment I've added.

Dec 14 2022, 7:45 AM · Restricted Project, Restricted Project
jmorse accepted D140006: fix jump threading failing to update cloned dbg.values.

LGTM -- let's leave it a while to give more opportunity for input.

Dec 14 2022, 7:17 AM · Restricted Project, Restricted Project
jmorse added a comment to D140006: fix jump threading failing to update cloned dbg.values.

According to the style guide, "Variable names should [...] be camel case, and start with an upper case letter (e.g. Leader or Boats).", you'll need to update the variable names you've added. Otherwise all looks good.

Dec 14 2022, 6:40 AM · Restricted Project, Restricted Project

Dec 9 2022

jmorse accepted D136320: [Assignment Tracking Analysis][1/*] Add analysis pass core.

Latest two revisions look good -- except that you've not merged them together when uploading them, so the latter overwrite the former!

Dec 9 2022, 6:58 AM · Restricted Project, Restricted Project, debug-info
jmorse accepted D136335: [Assignment Tracking Analysis][5/*] Tests.

I had another chat with Orlando about how we're testing these patches -- I'm pretty happy that we're now testing all the important paths through the assignment-tracking analysis pass, and the different ways that CFGs can compose and have different lattice input/output values. Nothing really springs to mind about what else we can be testing -- and significantly, this work is not going to be enabled by default, we'll be doing a decent amount of validating on larger projects first.

Dec 9 2022, 6:21 AM · Restricted Project, Restricted Project, debug-info
jmorse accepted D136325: [Assignment Tracking Analysis][3/*] Memory location fragment filling.

LGTM, with a request for some more documentation comments in the earlier comment.

Dec 9 2022, 5:02 AM · Restricted Project, Restricted Project, debug-info
jmorse added a comment to D136325: [Assignment Tracking Analysis][3/*] Memory location fragment filling.

I think I've clocked that this is a two-value lattice of \top and \bottom / true and false, indicating "this bit is in memory" or not in memory, and the intersection on meet ensures that it monotonically goes downwards, so we gradually remove bits from being in memory. Which sounds good and correct.

Dec 9 2022, 4:54 AM · Restricted Project, Restricted Project, debug-info

Dec 2 2022

jmorse added a comment to D136335: [Assignment Tracking Analysis][5/*] Tests.

The new two tests look good, and the documentation is highly appreciated -- is there are need to check for new "loc=none" locations being fed in, or is that something that's in the domain of LiveDebugValues? (I keep on switching back to the mindset of "every piece of information needs a dbg.value", which is not the situation in this pass).

Dec 2 2022, 8:22 AM · Restricted Project, Restricted Project, debug-info
jmorse added a comment to D138869: [Docs][RFC] Add AMDGPU LLVM Extensions for Heterogeneous Debugging.

Hi Scott,

Dec 2 2022, 4:19 AM · Restricted Project, Restricted Project

Nov 30 2022

jmorse added a reviewer for D138943: [WebAssembly] Use LiveDebugValues analysis: jmorse.

I see from the inline comments that virtual registers in WASM at this stage may or may not be meaningful -- if we can avoid LiveDebugValues having to encounter virtual registers, that's highly preferred, as it avoids considering a whole load of scenarios that LiveDebugValues isn't designed for. The number space for LocIndex is designed only for physregs and spill slots (physregs from 1 to 2^30, spill slots from 2^30+1 to 2^31-1). Adding the number range for virtual registers would require more testing.

Nov 30 2022, 8:28 AM · Restricted Project, Restricted Project
jmorse added a comment to D138992: [DebugInfo][SROA] Correct debug info for global variables spanning multiple fragments in case of SROA.

Before digging into the code, I'm not sure I understand the failing IR completely -- for the segment:

Nov 30 2022, 3:02 AM · Restricted Project, Restricted Project, debug-info
jmorse added a reviewer for D138992: [DebugInfo][SROA] Correct debug info for global variables spanning multiple fragments in case of SROA: Orlando.
Nov 30 2022, 2:50 AM · Restricted Project, Restricted Project, debug-info

Nov 25 2022

jmorse added a comment to D136335: [Assignment Tracking Analysis][5/*] Tests.

I suppose another way of thinking about that is ensuring every lattice transition gets explored across every CFG join pattern that can exist.

Nov 25 2022, 4:33 AM · Restricted Project, Restricted Project, debug-info
jmorse added a comment to D136335: [Assignment Tracking Analysis][5/*] Tests.

Just thinking out loud about coverage -- it seems to me that there are several dimensions over which the assignment-tracking lattice analysis should run, which are:

  • The domain of dbg.assign "state"/value, i.e. whether it has complete information in the intrinsic: {`both, value-only, link-only, neither},
  • Whether there's a linked store or not,
  • Whether the store comes before or after the intrinsic,
  • CFG patterns between the store, intrinsic, and intrinsics with a different state/value, i.e. whether there's straight-line-code, diamonds, loops, in the way.
Nov 25 2022, 4:33 AM · Restricted Project, Restricted Project, debug-info
jmorse accepted D133296: [Assignment Tracking][13/*] Account for assignment tracking in SROA.

LGTM, note you've added a 'tmp.ll' that you probably didn't intend to? Best to remove that.

Nov 25 2022, 3:57 AM · Restricted Project, Restricted Project, debug-info

Nov 24 2022

jmorse added a comment to D136335: [Assignment Tracking Analysis][5/*] Tests.

I can't really work out what untagged-store-frag.ll is testing as I don't get the notation, alas.

Nov 24 2022, 5:24 AM · Restricted Project, Restricted Project, debug-info
jmorse added a comment to D136320: [Assignment Tracking Analysis][1/*] Add analysis pass core.

Looks good with some nits fixed, a spurious return removed, and the question on line 825 addressed.

Nov 24 2022, 4:14 AM · Restricted Project, Restricted Project, debug-info
jmorse accepted D133118: Fix invalid llvm.dbg.declare after instcombine (#56807).

LGTM, thanks for keeping on this.

Nov 24 2022, 2:11 AM · Restricted Project, Restricted Project

Nov 23 2022

jmorse added a comment to D136335: [Assignment Tracking Analysis][5/*] Tests.

Worth putting implicit-check-not on all the tests?

Nov 23 2022, 7:49 AM · Restricted Project, Restricted Project, debug-info
jmorse updated subscribers of D133118: Fix invalid llvm.dbg.declare after instcombine (#56807).

Looks good -- the change to alloca-bitcast.ll is suspicious (an undef dbg.assign becoming valued again) but that probably signals an assignment tracking issue. CC @Orlando, what should happen here?

Nov 23 2022, 2:48 AM · Restricted Project, Restricted Project

Nov 22 2022

jmorse requested changes to D136325: [Assignment Tracking Analysis][3/*] Memory location fragment filling.

This is well motivated and useful -- I'm having some trouble building a mental model of what the lattice looks like though. IIUC we're seeking to identify the ranges of variable location that are well defined by the fragment ranges, but, because we later drop overlapped ranges at a later date, we want to explicitly state the not-overlapped memory location so that it isn't dropped. I don't quite see how meetFragments achieves that, as it only selects the bits that do overlap. Is the lattice collecting all the ranges where there's genuine overlap, so that later dbg.values split their ranges across the intersection? And if so, why does addDef delete "fully contained" overlaps, shouldn't it break all definitions up?

Nov 22 2022, 7:50 AM · Restricted Project, Restricted Project, debug-info
jmorse accepted D136321: [Assignment Tracking Analysis][2/*] Remove redundant location definitions.

This is all fine and well; are redundant variable locations a common case and which should we optimise for? IIUC every instruction with debug-info results in a SmallVector copy right now, which isn't necessary if that specific position hasn't had debug-info change. IMO, adding a "ChangedPosition" flag that's used to skip setWedge if it isn't true would be beneficial.

Nov 22 2022, 4:30 AM · Restricted Project, Restricted Project, debug-info

Nov 21 2022

jmorse requested changes to D136320: [Assignment Tracking Analysis][1/*] Add analysis pass core.

(Ticking "request changes" to take it out of the review queue)

Nov 21 2022, 3:42 AM · Restricted Project, Restricted Project, debug-info
jmorse requested changes to D133296: [Assignment Tracking][13/*] Account for assignment tracking in SROA.

(Ticking "request changes" to take it out of the review queue)

Nov 21 2022, 3:38 AM · Restricted Project, Restricted Project, debug-info

Nov 20 2022

jmorse added inline comments to D136320: [Assignment Tracking Analysis][1/*] Add analysis pass core.
Nov 20 2022, 1:27 PM · Restricted Project, Restricted Project, debug-info
jmorse added a comment to D136320: [Assignment Tracking Analysis][1/*] Add analysis pass core.

I'm in as far as line 900, hitting submit to clear the queue of comments just in case I don't get back to this. Random remarks:

Nov 20 2022, 12:50 PM · Restricted Project, Restricted Project, debug-info

Nov 17 2022

jmorse added inline comments to D133307: [Assignment Tracking][14/*] Account for assignment tracking in instcombine.
Nov 17 2022, 7:53 AM · Restricted Project, Restricted Project, debug-info
jmorse accepted D133321: [Assignment Tracking][24/*] Always RemoveRedundantDbgInstrs in instcombine in assignment tracking builds.

LGTM

Nov 17 2022, 7:19 AM · Restricted Project, Restricted Project, debug-info
jmorse accepted D133318: [Assignment Tracking][21/*] Account for assignment tracking in inliner.

LGTM with a couple of suggestions. The bigger comment is me asking "have you considered what happens when the inliner deletes random elements of assignment tracking due to pruning" -- I don't think there's anything that would be broken, just checking that we have a common understanding.

Nov 17 2022, 6:25 AM · Restricted Project, Restricted Project, debug-info
jmorse added inline comments to D133296: [Assignment Tracking][13/*] Account for assignment tracking in SROA.
Nov 17 2022, 5:26 AM · Restricted Project, Restricted Project, debug-info
jmorse accepted D133310: [Assignment Tracking][15/*] Account for assignment tracking in simplifycfg.

LGTM, with one or two questions, and the middle test needs -simplifycfg adding

Nov 17 2022, 3:50 AM · Restricted Project, Restricted Project, debug-info
jmorse accepted D133307: [Assignment Tracking][14/*] Account for assignment tracking in instcombine.

LGTM

Nov 17 2022, 3:03 AM · Restricted Project, Restricted Project, debug-info
jmorse added a comment to D133118: Fix invalid llvm.dbg.declare after instcombine (#56807).

Reverse-ping -- would this be landable for the next release? The test revisions needed are minor.

Nov 17 2022, 2:55 AM · Restricted Project, Restricted Project

Nov 16 2022

jmorse added a comment to D133296: [Assignment Tracking][13/*] Account for assignment tracking in SROA.

Looks good, although I think some of the logic at one of the inline comments can be simplified.

Nov 16 2022, 9:13 AM · Restricted Project, Restricted Project, debug-info
jmorse accepted D133294: [Assignment Tracking][11/*] Update RemoveRedundantDbgInstrs.

LGTM with some cosmetic nits.

Nov 16 2022, 4:01 AM · Restricted Project, Restricted Project, debug-info
jmorse added a comment to D135714: [MemProf] ThinLTO summary support.

Hi Teresa,

Nov 16 2022, 3:27 AM · Restricted Project, Restricted Project