[DebugInfo] Stop relying on asm gadgets to extend variable ranges #29029
Conversation
|
@swift-ci smoke test |
|
I'll add that for the specific case of rdar://57754659, I tested a change in LiveDebugValues that would preserve the two dropped variables without requiring any changes to Swift IRGen. However, the change has a significant compile-time impact for very little pay-off. E.g. variable scope coverage in an optimized kernel build is unaffected by the change. |
There was a problem hiding this comment.
This is trading one class of bugs against another, but this is also how Clang works at -O0, so I'm on board with this change. We might even be able to describe the values with dbg.addr/dbg.value instead of dbg.declare and thus reclaim the precise start point in some cases.
lib/IRGen/IRGenSIL.cpp
Outdated
| return (IGM.DataLayout.getTypeSizeInBits(Storage->getType()) > | ||
| IGM.getClangASTContext().getTargetInfo().getRegisterWidth()) || | ||
| isa<llvm::PHINode>(Storage); | ||
| return !isa<llvm::Constant>(Storage) || |
lib/IRGen/IRGenSIL.cpp
Outdated
| isa<llvm::PHINode>(Storage); | ||
| return !isa<llvm::Constant>(Storage) || | ||
| (IGM.DataLayout.getTypeSizeInBits(Storage->getType()) > | ||
| IGM.getClangASTContext().getTargetInfo().getRegisterWidth()); |
There was a problem hiding this comment.
but this condition, I'm not sure of
There was a problem hiding this comment.
I would expect all non-constants to require a shadow copy?
There was a problem hiding this comment.
All non-constants do get a shadow copy, I left the register-width condition in place to handle constants that don't fit in a register. However it looks like swift IRgen doesn't produce this kind of constant, so I'll just delete the condition.
There was a problem hiding this comment.
Sorry I misread that. We cannot express constants > 64bits in LLVM IR. A 80-bit float or a 128-bit vector would violate that requirement.
There was a problem hiding this comment.
For swift, I think it's safe to remove the register-width check, based on a quick skim of GenConstant/IRGenSIL.
In the past though, clang has emitted e.g. i65 integers to implement overflow checks. In some cases it might still use i129 etc.
The 'fake use' asm gadget does not always keep variables alive. E.g., in rdar://57754659, llvm was unable to preserve two local variables despite the use of these gadgets. These variables were backed by a LoadInst and an ExtractValueInst respectively. Instead of emitting shadow copies for just those kinds of instructions, use shadow copies exclusively. This may cause more variables to appear in the debugger window before they are initialized, but should result in fewer variables being dropped. rdar://57754659
|
@swift-ci smoke test |
|
Borked change was reverted recently, so I'm re-running the test. @swift-ci please smoke test |
|
@varungandhi-apple Thanks! |
|
@adrian-prantl did you have any other concerns on this one? |
The 'fake use' asm gadget does not always keep variables alive. E.g., in
rdar://57754659, llvm was unable to preserve two local variables despite
the use of these gadgets. These variables were backed by a LoadInst and
an ExtractValueInst respectively. Instead of emitting shadow copies for
just those kinds of instructions, use shadow copies exclusively. This
may cause more variables to appear in the debugger window before they
are initialized, but should result in fewer variables being dropped.
rdar://57754659