This is an archive of the discontinued LLVM Phabricator instance.

[Assignment Tracking] getIntrinsicInstrCost: set dbg.assign cost to zero
ClosedPublic

Authored by Orlando on Feb 23 2023, 9:09 AM.

Details

Summary

Does anyone have any good ideas on what a test for this looks like?

Diff Detail

Event Timeline

Orlando created this revision.Feb 23 2023, 9:09 AM
Herald added a project: Restricted Project. ยท View Herald TranscriptFeb 23 2023, 9:09 AM
Orlando requested review of this revision.Feb 23 2023, 9:09 AM
Herald added a project: Restricted Project. ยท View Herald TranscriptFeb 23 2023, 9:09 AM
Herald added a subscriber: llvm-commits. ยท View Herald Transcript
Orlando added a subscriber: jryans.Feb 23 2023, 9:12 AM

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

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

Orlando updated this revision to Diff 503306.Mar 8 2023, 3:46 AM

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โ€“13

Example of the cost being 1 for a dbg.assign, which would be wrong?

Orlando added inline comments.Mar 13 2023, 4:02 AM
llvm/test/Analysis/CostModel/X86/free-intrinsics.ll
11โ€“13

Aha yes, must have introduced this mistake while fiddling with D145573. Are you happy for me to just fix this before landing?

jmorse accepted this revision.Mar 13 2023, 4:42 AM

LGTM with inline rider

llvm/test/Analysis/CostModel/X86/free-intrinsics.ll
11โ€“13

Sure, that sounds alright.

This revision is now accepted and ready to land.Mar 13 2023, 4:42 AM
This revision was landed with ongoing or failed builds.Mar 14 2023, 2:26 AM
This revision was automatically updated to reflect the committed changes.