Page MenuHomePhabricator

[Assignment Tracking Analysis][5/*] Tests
Needs ReviewPublic

Authored by Orlando on Oct 20 2022, 4:06 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

The tests all check that the resultant MIR debug instructions look right when using SelectionDAG with AssignmentTrackingAnalysis.


Notes

I've bundled the tests together because the "core", "redundant removal", and "fragment fill" parts of the pass were not written in isolation as they are presented. I suppose the tests could be separated out into the patches if D136331 is applied before D136321 and D136325, but some disentangling will be required so it is my preference to keep them bundled if possible.

Maybe there is a better way to test the analysis that doesn't rely on something interpreting the results? OTOH, we do also want to be sure the results are being interpreted sensibly.

I'm definitely open to adding more tests if anyone has any suggestions.

Diff Detail

Event Timeline

Orlando created this revision.Oct 20 2022, 4:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2022, 4:06 AM
Herald added subscribers: ormris, zzheng. · View Herald Transcript
Orlando requested review of this revision.Oct 20 2022, 4:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2022, 4:06 AM

Some minor comments, but otherwise LGTM.

llvm/test/DebugInfo/assignment-tracking/X86/diamond-1.ll
29
47
llvm/test/DebugInfo/assignment-tracking/X86/diamond-2.ll
17
35
llvm/test/DebugInfo/assignment-tracking/X86/diamond-3.ll
20–22
llvm/test/DebugInfo/assignment-tracking/X86/loop-hoist.ll
21
34

Are the variables other than "a" actually needed for this test? Usually I think we'd delete variables that aren't relevant to the behaviour being tested.

llvm/test/DebugInfo/assignment-tracking/X86/loop-sink.ll
30–31

Could you try and test this with a vreg that isn't dropped? Checking for $noreg means that the expected behaviour is only being half tested for imo; if this particular "use a DbgDef instead of a MemDef" branch had a bug and just dropped defs instead of using the correct Value, this test wouldn't pick it up.

43

Same comment as the above test, if the variables other than "a" aren't being tested for could they be deleted from the test?

llvm/test/DebugInfo/assignment-tracking/X86/lower-to-value.ll
9

Add an implicit-check-not for DBG_INSTR_REF for the experiemental-debug-locations case?

47–48

More just a question for my understanding than anything relevant to the test, but as far as I can tell continuing to use a stack location for the [0, 64] fragment would be correct in this case - is a partial invalidation of the stack location pessimistically considered to invalidate the whole stack location in some cases, and if so what makes it different from the frag-agg case (where we do redeclare the stack location for the valid bytes) in the next test?

llvm/test/DebugInfo/assignment-tracking/X86/mem-loc-frag-fill-cfg.ll
12
48

Nit, but I don't think this needs to be a CHECK-DAG, the metadata section comes before the MIR contents of the file iirc.

llvm/test/DebugInfo/assignment-tracking/X86/mem-loc-frag-fill.ll
9
32

Ditto above, no need for -DAG

llvm/test/DebugInfo/assignment-tracking/X86/no-redundant-def-after-alloca.ll
4
6
llvm/test/DebugInfo/assignment-tracking/X86/untagged-store-frag.ll
10

Just for formatting consistency, maybe include the "Use X loc" here.

14
30–32

With these adjacent DBG_VALUEs describing the same variable, if the "remove redundant debug values" step understands that the first DBG_VALUE is entirely covered by the following two fragments then this test would start to fail. Maybe add an additional instruction between the first dbg.assign and dbg.value so that these two are not redundant (even if they currently aren't recognized as being so)? AFAIK that might also result in an extra debug value being produced by the agg-frag step, but I don't think there's any problem with that being included in the test if so.

llvm/test/DebugInfo/assignment-tracking/X86/use-known-value-at-early-mem-def-2.ll
6
44–46

Just to confirm - we're aware that c = 5, but because we're trying to produce an implicit value for the fragment it would take a bit of work to confirm what the lower 32 bits actually are? Which I think (assuming we don't examine the constant itself - also this is not a request to implement as part of this patch stack) would require us to insert an AND into the DIExpression to pull out only the lower 32 bits of the implicit value, i.e. DBG_VALUE 5, $noreg, ![[var]], !DIExpression(DW_OP_constu 0xffffffff, DW_OP_and, DW_OP_stack_value). Does that sound correct?

Orlando planned changes to this revision.Tue, Nov 15, 2:01 AM
Orlando updated this revision to Diff 475832.Wed, Nov 16, 8:20 AM
Orlando marked 19 inline comments as done.

Thanks for another eagle-eyed review @StephenTozer

llvm/test/DebugInfo/assignment-tracking/X86/lower-to-value.ll
47–48

as far as I can tell continuing to use a stack location for the [0, 64] fragment would be correct in this case

That's right. We don't need to redeclare that the [0, 64) fragment is on the stack because the [64, 128) fragment loc def doesn't overlap the previous loc def for [0, 64), and non-overlapping fragment locations compose properly (i.e., mem-loc-frag-fill doesn't need to do anything here because it already works as expected).

llvm/test/DebugInfo/assignment-tracking/X86/untagged-store-frag.ll
30–32

f the "remove redundant debug values" step understands that the first DBG_VALUE is entirely covered by the following two fragments

IMO it'd be better to update the test if/when that happens - it feels like slightly unnecessary future-proofing to me. wdyt?

The important parts of the test are steps 4 (value/value) and 5 (mem/value after untagged store to lower bits).

llvm/test/DebugInfo/assignment-tracking/X86/use-known-value-at-early-mem-def-2.ll
44–46

Yeah that's right

This can still be reviewed by others because the changes are not intrusive, but I plan to:

  • Update the tests to use opaque pointers
  • Remove TBAA metadata
Orlando updated this revision to Diff 476040.Thu, Nov 17, 1:09 AM

Removed tbaa metadata & updated tests to use opaque pointers.

Changes all LGTM, except the one unaddressed comment about not using $noreg in loop-sink.ll.

llvm/test/DebugInfo/assignment-tracking/X86/untagged-store-frag.ll
30–32

Sure - at least it wouldn't be a "silent" failure, and if someone is modifying the redundant debug values code it shouldn't be hard to identify the cause of this failure. Though it also wouldn't hurt for posterity to add a comment along the lines of "TODO: If removeRedundantDbgInstrs gets smarter, add an instruction here to preserve this set of debug values".

jmorse added a subscriber: jmorse.Wed, Nov 23, 7:49 AM

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

(got up to loop-unroll.ll)

llvm/test/DebugInfo/assignment-tracking/X86/dbg-phi-produces-undef.ll
5–6

What would happen if non-constants were used, but there were no out-of-block users? The reason being that today we can recover variable locations that are coincidentally PHI'd correctly due to being in the right registers, even if there isn't an IR PHI happening and being used. But that won't happen if an explicit DBG_VALUE $noreg is produced at the merge point.

24

Nit -- something other than literal "XXX" would be preferred, I don't know whether this matches other peoples styles, but I've only ever seen XXX used to indicate TODOs or sketchy situations. In the default vim syntax highlighting config, XXX is highlighted in yellow as a TODO.

llvm/test/DebugInfo/assignment-tracking/X86/diamond-3.ll
34–37

I endorse the syntax; if this isn't happening, isn't that a regression from LowerDbgDeclare? debug-info-on-edges is a serious problem that we don't have a solution for right now sadly.

llvm/test/DebugInfo/assignment-tracking/X86/loop-hoist.ll
10–19

Just thinking out loud -- I suppose that without assignment tracking, we would end up with one dbg.value in the entry block for the argument, then one in the middle of the do.body loop. And because where the do.body loop joins there's no PHI to merge those values, there's no way to get a correct variable location on the multiplication.

I don't see any way to improve this with assignment tracking, right? It always requires there to be a PHI, so that we know which loop iteration we're in.

(Not a criticism, just confirming to myself that I understand what's going on).

llvm/test/DebugInfo/assignment-tracking/X86/loop-sink.ll
30–31

Looks like it's because it's an Argument rather than any other value -- I agree that it'd be better to fiddle with the test to avoid checking for undef, this will ensure good coverage in the future.

58

I had the slight expectation that this was going to get a dbg.value attached to it, this is deliberately dropped then?

Orlando updated this revision to Diff 477526.Wed, Nov 23, 9:14 AM
Orlando marked 3 inline comments as done.

Address / respond to inline comments.

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

NOTE to self: look into this.

(forgot to submit the inline comments)

llvm/test/DebugInfo/assignment-tracking/X86/dbg-phi-produces-undef.ll
5–6

Interesting question -- the DBG_VALUE $noreg is not actually produced at the merge point though, it's inserted after the store which is storing performing assignment named !2 while the debug program asserts that assignment !1 should still be visible.

AssignmentTrackingAnalysis just tracks merge points, it does not insert location defs (/undef) at merge points itself.

llvm/test/DebugInfo/assignment-tracking/X86/diamond-3.ll
34–37

Yeah it is a regression. I think TODO / WISHLIST should probably be renamed FIXME to indicate this. This is on my mental TODO list but I should add this to the actual TODO list in the documentation - leaving this comment as "not done" as a reminder to do that before landing.

llvm/test/DebugInfo/assignment-tracking/X86/loop-hoist.ll
10–19

Yeah that all sounds correct.

llvm/test/DebugInfo/assignment-tracking/X86/loop-sink.ll
30–31

Sorry I left this one not-done to come back to, and forgot to come back to it. Done.

58

As in by the analysis pass (as a DBG_VALUE/INSTR_REF), or in this IR? Wasn't there a patch that stopped insertDebugValuesForPHIs being called by LCSSA maybe a year or so ago?

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

llvm/test/DebugInfo/assignment-tracking/X86/diamond-3.ll
34–37

Cool; I think we're alright landing things + evaluating so long as we've got this limitation on the radar. It's not like LowerDbgDeclare is perfect anyway.

llvm/test/DebugInfo/assignment-tracking/X86/loop-sink.ll
58

Yeah, but IIRC that was only for PHIs with single operands -- the loop here doesn't dominate the exit block, thus the PHI is relevant. Prodding this (full marks for including original source) with clang-15 shows it's not a regression. I'll write this down as something to examine later I guess. I think in the general case LiveDebugValues will recover this anyway.

llvm/test/DebugInfo/assignment-tracking/X86/order-of-defs.ll
12

!20 needs to be a captured variable

llvm/test/DebugInfo/assignment-tracking/X86/remove-redundant-defs-to-prevent-reordering.ll
21

Not sure if it's "A" or "An", depends how you pronounce it I guess

llvm/test/DebugInfo/assignment-tracking/X86/remove-undef-fragment.ll
73

You'll make the undef-to-poison people happy if you don't add this in a test.

llvm/test/DebugInfo/assignment-tracking/X86/untagged-store-frag.ll
5–6

From a standing start, I don't know what mem(a) and dbg(a) represent -- presumably the in-memory-or-in-value result computed by assignment tracing? But then the comment on 2. conflicts with that?

llvm/test/DebugInfo/assignment-tracking/X86/use-known-value-at-early-mem-def.ll
6

(just responding to one inline comment to try to clear up my confusing comments)

llvm/test/DebugInfo/assignment-tracking/X86/untagged-store-frag.ll
5–6

1. mem(a): bits [0, 64) = !14 represents a store to the memory location of a (bits [0, 64)) with a !DIAssignID !14 attachment .
2. dbg(a): bits [0, 64) = !14 represents a for the same var/bits using !DIAssignID !14

The end-of-line comment indicates what the analysis is working out. The analysis sees that at (/just after) position 2 the (conceptual/source) variable and the variable's memory location are both dominated by the assignment !14 at this point, so we can use the memory location. Does that help at all?

Orlando added inline comments.Thu, Nov 24, 6:04 AM
llvm/test/DebugInfo/assignment-tracking/X86/untagged-store-frag.ll
5–6

2. dbg(a): bits [0, 64) = !14 represents a for the same var/bits using !DIAssignID !14 is meant to read "...represents a dbg.assign for the same...

Sorry!

Orlando updated this revision to Diff 477894.Fri, Nov 25, 2:27 AM
Orlando marked 6 inline comments as done.
Orlando added inline comments.
llvm/test/DebugInfo/assignment-tracking/X86/order-of-defs.ll
12

Removed - I don't think they add anything valuable to the test.

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.

Put it all together and you get something like 50 or 60 different scenarios to test. We don't need explicit tests for every single path through the compiler, however to my mind what we're missing right now is interactions between loops and dbg.assigns with different states. This is something that LiveDebugValues used to get very wrong. For example, IMO we want an explicit test like loop-sink.ll but where there's a second loop exit path where there's a dbg.value(undef,). In that situation we'd want to ensure that the subsequent store doesn't become the variable location.

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