This is an archive of the discontinued LLVM Phabricator instance.

[Assignment Tracking Analysis][5/*] Tests
ClosedPublic

Authored by Orlando on Oct 20 2022, 4:06 AM.

Details

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
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
32

Ditto above, no need for -DAG

StephenTozer added inline comments.Nov 4 2022, 6:32 AM
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?

llvm/test/DebugInfo/assignment-tracking/X86/mem-loc-frag-fill.ll
9
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.Nov 15 2022, 2:01 AM
Orlando updated this revision to Diff 475832.Nov 16 2022, 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.Nov 17 2022, 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.Nov 23 2022, 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.Nov 23 2022, 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
11

!20 needs to be a captured variable

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

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
72

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
4–5

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
5

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

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

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.Nov 24 2022, 6:04 AM
llvm/test/DebugInfo/assignment-tracking/X86/untagged-store-frag.ll
4–5

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.Nov 25 2022, 2:27 AM
Orlando marked 6 inline comments as done.
Orlando added inline comments.
llvm/test/DebugInfo/assignment-tracking/X86/order-of-defs.ll
11

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.

Orlando updated this revision to Diff 478968.Nov 30 2022, 8:38 AM

+ Add nested-loop.ll that tests a variety of inputs/lattices

Note that this actually currently fails (just the part for variable 'f') - the test uncovered a bug with untagged store handling. I thought it would be worth getting the test up for review anyway.

If you're happy with this style of test then I'll add another that uses fragments to check that all works too.

+ Add nested-loop.ll that tests a variety of inputs/lattices

Note that this actually currently fails (just the part for variable 'f') - the test uncovered a bug with untagged store handling. I thought it would be worth getting the test up for review anyway.

If you're happy with this style of test then I'll add another that uses fragments to check that all works too.

I've fixed the analysis patch so that this test now passes.

Orlando updated this revision to Diff 479314.Dec 1 2022, 8:52 AM

Add nested-loop-sroa.ll which is a copy of nested-loop.ll except that the allocas hold a fragment of a variable except the whole variable. In writing this I discovered another bug with untagged store handling (I introduced it in my last update addressing the previous bug).

jmorse added a comment.Dec 2 2022, 8:22 AM

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).

llvm/test/DebugInfo/assignment-tracking/X86/nested-loop-sroa.ll
89–97 ↗(On Diff #479314)

To help my understanding -- the dbg=phi on entry to do.body is because entry feeds in constant-value 9, while the assignment in do.body1 feeds in constant-value 8, yes? While stores of !77 to the stack location dominate from entry onwards.

Orlando added inline comments.Dec 6 2022, 3:12 AM
llvm/test/DebugInfo/assignment-tracking/X86/nested-loop-sroa.ll
89–97 ↗(On Diff #479314)

the dbg=phi on entry to do.body is because entry feeds in constant-value 9, while the assignment in do.body1 feeds in constant-value 8, yes

Yes except it's the DIAssignIDs associated with those constant-value dbg.assigns being different that matters, rather than the constant values. The 8 and 9 could both be 8, for example, and the table would look the same.

While stores of !77 to the stack location dominate from entry onwards.

Yep

160 ↗(On Diff #479314)

Note to self: the end of line comment above should be VAR:e rather than VAR:c.

is there are need to check for new "loc=none" locations being fed in

The two new tests both do encounter None in process but I think you're right that neither check that None is joined correctly. I'll add another variable to them both to check that.

FYI I'm working on one final test that exercises the mem-loc-frag-fill pass too.

Orlando updated this revision to Diff 480871.Dec 7 2022, 5:45 AM

+ Fix comments in tests
+ Add nested-loop-frags.ll which is similar to nested-loop-sora.ll and nested-loop.ll. It has 64 bit variables/allocas, with a mix of 32 and 64 bit stores.

Another bug was found when writing that new test which I will patch shortly. All of these bugs have been to do with fragment handling (unsurprisingly).

jmorse accepted this revision.Dec 9 2022, 6:21 AM

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.

With that in mind, LGTM to land. I anticipate that validation is going to find other bugs in this pass, but we're past the point of more guessing-at-what-to-test being a good return on investment....

Another bug was found when writing that new test which I will patch shortly. All of these bugs have been to do with fragment handling (unsurprisingly).

Assuming this is now fixed!

This revision is now accepted and ready to land.Dec 9 2022, 6:21 AM
Orlando marked 2 inline comments as done.Dec 9 2022, 9:09 AM
Orlando added inline comments.
llvm/test/DebugInfo/assignment-tracking/X86/diamond-3.ll
34–37
saghir added a subscriber: saghir.Dec 9 2022, 11:34 AM

Hi @Orlando, the test case (remove-undef-fragment.ll) that you added as part of 1d1de7467c32d52926ca56b9167a2c65c451ecfa is breaking one of our bots: https://lab.llvm.org/staging/#/builders/14/builds/1332. Can you please take a look? Thanks!

stefanp added a subscriber: stefanp.Dec 9 2022, 2:48 PM

It looks like the test test/DebugInfo/assignment-tracking/X86/remove-undef-fragment.ll fails on multiple PPC bots.
https://lab.llvm.org/buildbot/#/builders/214/builds/4756
https://lab.llvm.org/buildbot/#/builders/231/builds/6006
https://lab.llvm.org/buildbot/#/builders/121/builds/26032

The test only has two checks:

; CHECK: DBG{{.*}}DIExpression(DW_OP_LLVM_fragment, 0, 32)
; CHECK: DBG{{.*}}DIExpression(DW_OP_LLVM_fragment, 64, 32)

On PPC the output from ISel only has the second line. Was this test meant to be X86 specific? (I'm asking based on the path).
I'm inclined to restrict this test to X86 until we figure out the problem.

haowei added a subscriber: haowei.Dec 9 2022, 3:08 PM

Test "DebugInfo/assignment-tracking/X86/remove-undef-fragment.ll" is failing on Fuchsia's Linux ARM64 Clang builder as well. Error message:

Script:
--
: 'RUN: at line 1';   /b/s/w/ir/x/w/staging/llvm_build/bin/llc /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/DebugInfo/assignment-tracking/X86/remove-undef-fragment.ll -o - -stop-after=finalize-isel     -experimental-assignment-tracking   | /b/s/w/ir/x/w/staging/llvm_build/bin/FileCheck /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/DebugInfo/assignment-tracking/X86/remove-undef-fragment.ll --implicit-check-not=DBG
--
Exit Code: 1

Command Output (stderr):
--
/b/s/w/ir/x/w/llvm-llvm-project/llvm/test/DebugInfo/assignment-tracking/X86/remove-undef-fragment.ll:26:10: error: CHECK: expected string not found in input
; CHECK: DBG{{.*}}DIExpression(DW_OP_LLVM_fragment, 0, 32)
         ^
<stdin>:1:1: note: scanning from here
--- |
^
<stdin>:2:80: note: possible intended match here
 ; ModuleID = '/b/s/w/ir/x/w/llvm-llvm-project/llvm/test/DebugInfo/assignment-tracking/X86/remove-undef-fragment.ll'
                                                                               ^

Input file: <stdin>
Check file: /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/DebugInfo/assignment-tracking/X86/remove-undef-fragment.ll

-dump-input=help explains the following input dump.

Input was:
<<<<<<
            1: --- | 
check:26'0     X~~~~~ error: no match found
            2:  ; ModuleID = '/b/s/w/ir/x/w/llvm-llvm-project/llvm/test/DebugInfo/assignment-tracking/X86/remove-undef-fragment.ll' 
check:26'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
check:26'1                                                                                    ?                                      possible intended match
            3:  source_filename = "/b/s/w/ir/x/w/llvm-llvm-project/llvm/test/DebugInfo/assignment-tracking/X86/remove-undef-fragment.ll" 
check:26'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            4:  target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128" 
check:26'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            5:   
check:26'0     ~~
            6:  define void @_Z1kv({ <2 x float>, <2 x float> } %call, <2 x float> %0, float %n.sroa.6.8.vec.extract) !dbg !7 { 
check:26'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            7:  entry: 
check:26'0     ~~~~~~~~
            .
            .
            .
>>>>>>

builder link: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-arm64/b8795235652012316209/overview

Please take a look and if it takes a while to fix, please revert this patch.

I've made the test X86 only to allow time for this to be investigated.
34a3259fab86aaa1a20224e08849775f3593e6a3
This should help bring the bots back to green in the meantime.

Test "DebugInfo/assignment-tracking/X86/remove-undef-fragment.ll" is failing on Fuchsia's Linux ARM64 Clang builder as well. Error message:

Sorry, I didn't realize that this was failing on other platforms as well. If you need to revert this patch feel free to revert the patch that I added as well.

Orlando marked an inline comment as done.Dec 9 2022, 3:34 PM

I've made the test X86 only to allow time for this to be investigated.
34a3259fab86aaa1a20224e08849775f3593e6a3
This should help bring the bots back to green in the meantime.

Thank you for sorting this out. These failures came in out of work hours for me here - I just came back to check on this patch and you've already kindly unblocked the bots.

It looks like llvm/test/DebugInfo/assignment-tracking/X86/remove-undef-fragment.ll is missing a target triple - I fix this on Monday.

Thanks again.