Page MenuHomePhabricator

[CodeGenPrepare] Handle all debug calls in dupRetToEnableTailCallOpts()
ClosedPublic

Authored by jonpa on Tue, Jan 22, 5:54 AM.

Details

Summary

This patch makes sure that a debug value that is after the bitcast in dupRetToEnableTailCallOpts() is also skipped.

The reduced test case is from SPEC-2006 on SystemZ.

Diff Detail

Event Timeline

jonpa created this revision.Tue, Jan 22, 5:54 AM
vsk added a subscriber: vsk.Tue, Jan 22, 11:14 AM

Thanks for the patch. Could you please reduce the test case a bit further? There are kilobytes of debug info metadata here which don't appear necessary for reproducing the problem.

To start, I suggest running opt -strip -metarenamer -debugify on the IR input. That should still reproduce the issue.

test/CodeGen/SystemZ/debuginstr-cgp.mir
55

I'm a bit confused about this. The change was about dbg.value instructions following bitcasts, but here we have the dbg.value following a getelementptr. Perhaps I'm missing something?

jonpa updated this revision to Diff 183055.Wed, Jan 23, 12:57 AM
jonpa marked 2 inline comments as done.

Thanks for the patch. Could you please reduce the test case a bit further? There are kilobytes of debug info metadata > here which don't appear necessary for reproducing the problem.

To start, I suggest running opt -strip -metarenamer -debugify on the IR input. That should still reproduce the issue.

Ah, that's how you do it! :-)

Test case reduced.

test/CodeGen/SystemZ/debuginstr-cgp.mir
55

Ah.. that's what Bugpoint did. It seems that CodeGenPrepare replaced that GEP (with only 0 indexes) with a bitcast before reaching the point of interest. I replaced it with a Bitcast in the test case now for clarity.

The test case looks much better now, thanks.

Usually we include a short set of instructions in the test case on how to produce the IR, including the source code, in case the IR needs to be re-generated in response to a format change etc.

Other than that, LGTM.

jonpa added a comment.Thu, Jan 24, 6:45 AM

The test case looks much better now, thanks.

Usually we include a short set of instructions in the test case on how to produce the IR, including the source code, in case the IR needs to be re-generated in response to a format change etc.

Other than that, LGTM.

I was not aware of that... The Debug info is just all "dummy" which has just been copied-and-pasted into the test case. I'm afraid I think my .ll test case is gone at the moment. Do you really need me to regenerate it, or could it be accepted given its small size?

(I don't think reducing the source input is meaningful given that the debug-info is not "real", right?)

If that's the case a brief excerpt of the original source construct (C, C++ or otherwise) that was used to produce the IR along with a couple of command lines to produce the MIR file should be sufficient. It doesn't have to be exact, but just so that the poor soul who has to regenerate the IR at some point in the future has something to go on. Check out llvm/test/CodeGen/MIR/X86/branch-folder-with-label.mir for an example.

jonpa updated this revision to Diff 183559.Fri, Jan 25, 9:25 AM

I made a new reduced MIR test case from the original source file and this time included the IR as a comment. Hope that works.

vsk added a comment.Fri, Jan 25, 9:56 AM

I see a minor issue in the test, but the patch looks great to me otherwise. Thanks!

test/CodeGen/SystemZ/debuginstr-cgp.mir
51

Something might have went wrong during the test update -- I think Wolfgang's comment about the missing bitcast applies again?

jonpa updated this revision to Diff 183803.Mon, Jan 28, 12:13 AM

Test updated once again to use a bitcast in the IR also before CodeGenPrepare.

jonpa marked an inline comment as done.Mon, Jan 28, 12:14 AM
This revision is now accepted and ready to land.Mon, Jan 28, 11:10 AM
jonpa closed this revision.Tue, Jan 29, 1:05 AM

Thanks for review!
r352462