Does anyone have any good ideas on what a test for this looks like?
Details
Diff Detail
Event Timeline
Note that dbg_addr is also missing from this list. dbg_addr is in the process of being removed (cc @jryans) but maybe it's worth fixing in the mean time, since it's such a tiny fix? YMMV.
The result of having dbg_addr and dbg_assign missing from the list is that they're considered to cost 1 rather than 0, which changes optimisation decisions (unrolling, peeling, inlining, etc).
I'm hoping to finish up dbg_addr removal this weekend (though my time estimates are nearly always too optimistic).
I guess I'd say it's not worth adding dbg_addr here since (a) it's almost gone and (b) it's rarely seen in wild. I am fine with either approach though, it's of course easy to remove it from here as part of my patch stack if it does get added. 🙂
I guess I'd say it's not worth adding dbg_addr here since
SGTM - it has already gone this long without anyone caring about the bug, it can wait a little longer till dbg_addr ceases to exist.
Does anyone have any good ideas on what a test for this looks like?
I can see a test that I think is meant to test this function, in llvm/test/Analysis/CostModel there are 3 tests which test the cost of intrinsics (free-intrinsics-datalayout.ll, free-intrinsics-no_info.ll, and X86/free-intrinsics.ll). Those tests are using the auto-generated check scripts though, and it looks like the debug intrinsics in those scripts are being dropped (warning: ignoring debug info with an invalid version (0)), so the update script doesn't produce a check-line for them. Given that this is effectively a trivial extension to untested code I don't think the lack of test should block this script; but it'd be a welcome if you fixed the debug info version issue in those tests, added a debug assign instance, and reran the update scripts to produce an actual test output (as long as it is trivial to do so).
Nice spot @StephenTozer - I've fixed those tests in D145573 and updated them in this patch to add dbg.assign checks.
This looks good, but in the test outputs shouldn't the cost be zero? Auto-regenerated without rebuilding opt perhaps? (See inline comment)
llvm/test/Analysis/CostModel/X86/free-intrinsics.ll | ||
---|---|---|
11 | Example of the cost being 1 for a dbg.assign, which would be wrong? |
LGTM with inline rider
llvm/test/Analysis/CostModel/X86/free-intrinsics.ll | ||
---|---|---|
11 | Sure, that sounds alright. |
Example of the cost being 1 for a dbg.assign, which would be wrong?