This is an archive of the discontinued LLVM Phabricator instance.

[framelowering] Skip dbg values when getting next/previous instruction.
ClosedPublic

Authored by fhahn on Dec 12 2016, 2:43 PM.

Details

Summary

In mergeSPUpdates, debug values need to be ignored when getting the
previous element, otherwise debug data could have an impact on codegen.

In eliminateCallFramePseudoInstr, debug values after the erased element
could have an impact on codegen and should be skipped.

Closes PR31319 (https://llvm.org/bugs/show_bug.cgi?id=31319)

Diff Detail

Event Timeline

fhahn updated this revision to Diff 81146.Dec 12 2016, 2:43 PM
fhahn retitled this revision from to [framelowering] Skip dbg values when getting next/previous instruction..
fhahn updated this object.
fhahn added reviewers: mkuper, aprantl, MatzeB.
fhahn added subscribers: llvm-commits, gbedwell.
aprantl edited edge metadata.Dec 12 2016, 3:01 PM

In general this LGTM, but I wonder if we don't have / shouldn't introduce a more idiomatic way of skipping over DBG_VALUEs.

lib/Target/X86/X86FrameLowering.cpp
376

Would it make sense to introduce a better API for this? Skipping over all DBG_VALUEs is a pretty common operation.

test/CodeGen/X86/frame-lowering-debug-intrinsic-2.ll
21

should these be CHECK-NEXTs ?

fhahn added inline comments.Dec 12 2016, 3:41 PM
lib/Target/X86/X86FrameLowering.cpp
376

I think a better API for this would make sense. I'll have a look tomorrow and see there's an existing API for that or if a new API could be introduced.

fhahn updated this revision to Diff 81773.Dec 16 2016, 10:38 AM
fhahn edited edge metadata.

Updated patch to use skipDebugInstructions* functions introduced in D27782

fhahn updated this revision to Diff 82170.Dec 20 2016, 3:46 PM
fhahn marked an inline comment as done.

Use a few more CHECK-NEXT where possible.

@aprantl It would be great if you could have another look now that this patch is using generic functions to skip debug instructions.

aprantl accepted this revision.Dec 21 2016, 10:49 AM
aprantl edited edge metadata.

I think this looks good now. Thanks! (one comment inline)

lib/Target/X86/X86FrameLowering.cpp
383

Perhaps this would be easier to read if skipDebugInstructions* are on a subsequent line outside the :? expression?

This revision is now accepted and ready to land.Dec 21 2016, 10:49 AM
fhahn updated this revision to Diff 82403.Dec 23 2016, 2:40 AM
fhahn edited edge metadata.

Moved skipDebugInstructionsBack/Forward to separate lines.

fhahn marked an inline comment as done.Dec 23 2016, 2:41 AM

@aprantl thank you very much for taking another look!

fhahn closed this revision.Dec 23 2016, 3:45 AM
fhahn added a comment.Dec 23 2016, 4:42 AM

Unfortunately this patch caused the sanitizer-x86_64-linux-autoconf builder to fail ( http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/2829 )

I've reverted the patch and will look into why it caused the failure in the next days.

fhahn updated this revision to Diff 82535.Dec 27 2016, 3:13 AM

Updated eliminateCallFramePseudoInstr to return the next instruction even if it's a debug value, because the caller is responsible to replace frame index constants in debug values.

This fixes the buildbot regression.

fhahn added a comment.Dec 27 2016, 3:15 AM

Please let me know if I should create a new review for the patch, because this review is already closed and I don't know how I could reopen it.

mkuper reopened this revision.Dec 27 2016, 10:00 AM
mkuper edited edge metadata.

Reopened. :-)

This revision is now accepted and ready to land.Dec 27 2016, 10:00 AM
fhahn added a comment.Dec 29 2016, 2:27 PM

thank you very much :)

The new diff changed quite a bit so I would appreciate if somebody could take another look.

MatzeB accepted this revision.Jan 2 2017, 6:07 AM
MatzeB edited edge metadata.

LGTM with nitpicks.

lib/Target/X86/X86FrameLowering.cpp
2587–2588

This feels out of place in front of the function. (We should always have /// doxygen in front of the function, as this just describes just a small detail I would rather it next to the return. Though I find the call of whether to comment or not already close as the information is already given in the docu comment of TargetFrameLowering::eliminateCallFramePseudoInstr().

2597

How about InsertPos as a name?

fhahn updated this revision to Diff 83029.Jan 4 2017, 3:45 AM
fhahn edited edge metadata.
fhahn marked 2 inline comments as done.

Thank you very much for your comments @MatzeB!

fhahn closed this revision.Jan 4 2017, 4:19 AM