This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][InstrRef][X86] Instrument expanded DYN_ALLOCA_ instructions with instruction numbers
ClosedPublic

Authored by jmorse on Nov 23 2021, 9:21 AM.

Details

Summary

If we have a DYN_ALLOCA_* instruction, it will eventually be expanded to a stack probe and subtract-from-SP. Add debug-info instrumentation to X86FrameLowering::emitStackProbe so that it can redirect debug-info for the DYN_ALLOCA to the lowered stack probe.

In practice, this means putting an instruction number label either the call instruction to _chkstk for win32, or more commonly on the subtract from SP instruction. The two tests added cover both of these cases.

NB:

  • I don't think there are any paths with the Large code-model and 64-bit Windows that will use the NumCallOps number, because it's only win32 that uses _chkstck to modify SP,
  • There are other code paths from emitStackProbe that implement probes, such as emitStackProbeInline, but I believe they all deal with fixed-size allocations in the prologue, and so aren't of interest to lowered DYN_ALLOCA instructions

(Adding Craig as this touches X86 things)

Diff Detail

Event Timeline

jmorse created this revision.Nov 23 2021, 9:21 AM
jmorse requested review of this revision.Nov 23 2021, 9:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2021, 9:21 AM

Makes sense to me from a debug info perspective, and tests LGTM. I've added a couple of nits inline. It might be worth someone else having a quick look as I'm not familiar with the .cpps touched here.

llvm/lib/Target/X86/X86DynAllocaExpander.cpp
217

super nit: I think the style guide prefers braces around a single statement if if there is also a nested comment?

llvm/lib/Target/X86/X86FrameLowering.cpp
1044

nits: Do you need NumCallOps? Is it not the same as ModInst->getNumOperands() here?

And since ModInst == CI on this code path (right?) IMO it would be clearer to refer to CI directly here.

jmorse updated this revision to Diff 389527.Nov 24 2021, 9:52 AM

Avoid counting number of operands when we can just query the instruction

jmorse added inline comments.Nov 24 2021, 9:54 AM
llvm/lib/Target/X86/X86FrameLowering.cpp
1044

Updated to use getNumOperands. Also, apparently I couldn't count in the previous version, because now we need to subtract two (one for it being penultimate op, one for it being zero-based).

IMHO it's better to use ModInst -- it avoids having to think about the earlier control flow. YMMV?

This revision was not accepted when it landed; it landed in state Needs Review.Nov 30 2021, 4:08 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.