[SILOptimizer][DebugInfo] Preliminary support for DIExpression in SROA and Mem2Reg #38736

mshockwave · 2021-08-03T21:23:56Z

SROA and Mem2Reg now can leverage DIExpression -- op_fragment, more
specifically -- to generate correct debug info for optimized SIL. Some
important highlights:

The new swift::salvageDebugInfo, similar to llvm::salvageDebugInfo,
tries to restore / transfer debug info from a deleted instruction.
Currently I only implemented this for store instruction whose
destination is an alloc_stack value.
DCE now does not remove debug_value instruction whose associating
with a function argument SSA value that is not used elsewhere. Since
that SSA value will not disappear so we should keep the debug info.

NOTE: This PR is only for code review. It depends on and includes #38734 #38735 . Please merge this PR instead of the previous two.

mshockwave · 2021-08-03T21:25:59Z

@swift-ci please test

swift-ci · 2021-08-03T23:10:24Z

Build failed
Swift Test Linux Platform
Git Sha - cd762e63cfc52f1c6f0c21bf7e9cde7fbc40fd20

include/swift/SIL/SILCloner.h

adrian-prantl · 2021-08-03T23:38:42Z

include/swift/SILOptimizer/Utils/DebugOptUtils.h

@@ -76,6 +81,7 @@ eraseFromParentWithDebugInsts(SILInstruction *inst,
  // Just matching what eraseFromParentWithDebugInsts is today.
  if (nextII == inst->getIterator())
    ++nextII;
+  swift::salvageDebugInfo(inst);


Does this need to be part of the SROA PR or should this be in its own patch?

My original thought was that if salvageDebugInfo is put in a separate patch (or PR), there will be no use case or test there, which is a little weird. What do you think?

Could we write a standalone test using sil-opt that runs exactly one pass that would delete an instruction? If the answer is no, then let's just keep it.

Currently we're calling salvageDebugInfo inside InstructionDeleter, which will affect multiple Passes. We can first create a PR that only calls salvageDebugInfo in a single Pass then revise it to use InstructionDeleter in later PR...but I feel like it will be a little over-engineering

lib/IRGen/IRGenDebugInfo.cpp

lib/IRGen/IRGenSIL.cpp

lib/SIL/IR/SILBasicBlock.cpp

swift-ci · 2021-08-03T23:55:05Z

Build failed
Swift Test OS X Platform
Git Sha - cd762e63cfc52f1c6f0c21bf7e9cde7fbc40fd20

The `alloc_stack` instruction now can carry advanced debug info like source-variable-specific SIL location, type, and SIL DIExpression.

Debug variables that are marked 'implicit' on its `debug_value` instruction mean that they were generated by compiler. Optimizers are free to remove them (if it becomes a dead code, for instance) even in -Onone. Since they are barely used by users and keeping them might lead to incorrect IRGen results.

mshockwave · 2021-08-04T21:26:36Z

I just found that I might can split the changes regarding debug scope here to a separate PR. I'll update this accordingly later

mshockwave · 2021-08-04T22:17:56Z

I just found that I might can split the changes regarding debug scope here to a separate PR. I'll update this accordingly later

Since that part of changes don't really affect the feature here, I'll put it in another PR that is not depending on the current series of patches
@swift-ci please test

mshockwave · 2021-08-04T23:09:25Z

@swift-ci please test

swift-ci · 2021-08-04T23:26:03Z

Build failed
Swift Test OS X Platform
Git Sha - 0dc20ba963614494beea0d2d7a859eafc361e06d

swift-ci · 2021-08-05T00:04:59Z

Build failed
Swift Test OS X Platform
Git Sha - e9db7d742a339fd56d1ebcaf6022c632bf73a81d

mshockwave · 2021-08-05T00:09:43Z

I just found that I might can split the changes regarding debug scope here to a separate PR. I'll update this accordingly later

Since that part of changes don't really affect the feature here, I'll put it in another PR that is not depending on the current series of patches

I was wrong about that: Without the debug scope changes it will fail on a pretty common optimization pattern (specialization on implicit-generated enum functions) despite passing all tests, so I'm bringing them back and add corresponding test cases. I've also addressed all the feedbacks on that part before they were previously detached from this PR.
Sorry for this.

@swift-ci please test

swift-ci · 2021-08-05T00:17:23Z

Build failed
Swift Test Linux Platform
Git Sha - e9db7d742a339fd56d1ebcaf6022c632bf73a81d

adrian-prantl

Great, I think you've addressed all my concerns.

swift-ci · 2021-08-05T01:52:40Z

Build failed
Swift Test Linux Platform
Git Sha - ab126dd1bfa0ebc0aed6fb1d32f0c43521b791a9

swift-ci · 2021-08-05T02:27:07Z

Build failed
Swift Test OS X Platform
Git Sha - ab126dd1bfa0ebc0aed6fb1d32f0c43521b791a9

…A and Mem2Reg SROA and Mem2Reg now can leverage DIExpression -- op_fragment, more specifically -- to generate correct debug info for optimized SIL. Some important highlights: - The new swift::salvageDebugInfo, similar to llvm::salvageDebugInfo, tries to restore / transfer debug info from a deleted instruction. Currently I only implemented this for store instruction whose destination is an alloc_stack value. - Since we now have source-variable-specific SIL location inside a `debug_value` instruction (and its friends), this patch teaches SILCloner and SILInliner to remap the debug scope there in addition to debug scope of the instruction. - DCE now does not remove `debug_value` instruction whose associating with a function argument SSA value that is not used elsewhere. Since that SSA value will not disappear so we should keep the debug info.

mshockwave · 2021-08-06T00:37:31Z

So the failure on Linux buildbot was caused by uninitialized struct type information during IRGen. I think it only happens if a debug_value instruction is the first ever SIL instruction that references a certain struct type, in which case IRGen hasn't translated that type into its internal representation (e.g. RecordTypeInfo).

A more sophisticated solution will be asking IRGen to resolve the type when this happens. But since this scenario is pretty rare and I don't want to add more complexity into this PR, for now I simply bails out the (LLVM) debug intrinsic generation process if this occurs, similar to our current resolution on non-fixed type.

@swift-ci please test

adrian-prantl · 2021-08-06T15:19:36Z

A more sophisticated solution will be asking IRGen to resolve the type when this happens. But since this scenario is pretty rare and I don't want to add more complexity into this PR, for now I simply bails out the (LLVM) debug intrinsic generation process if this occurs, similar to our current resolution on non-fixed type.

Thanks for adding the comment explaining this to IRGenDebugInfo. LGTM!

Don't let debug_value instructions bail the optimization. This fixes a couple of performance regressions, which were introduced by adding more debug_value instructions (swiftlang#38736). rdar://82327743

This was referenced Aug 3, 2021

[DebugInfo][SIL] Introduce the 'implicit' attribute for debug variable #38735

Merged

[SIL][DebugInfo] Bring advanced debug info to alloc_stack #38734

Merged

mshockwave requested a review from adrian-prantl August 3, 2021 21:27

adrian-prantl reviewed Aug 3, 2021

View reviewed changes

mshockwave added 2 commits August 4, 2021 12:56

[SIL][DebugInfo] Bring advanced debug info to alloc_stack

2bb6498

The `alloc_stack` instruction now can carry advanced debug info like source-variable-specific SIL location, type, and SIL DIExpression.

mshockwave force-pushed the dev-sil-diexpr-optimize branch from cd762e6 to 0dc20ba Compare August 4, 2021 22:14

mshockwave force-pushed the dev-sil-diexpr-optimize branch from 0dc20ba to e9db7d7 Compare August 4, 2021 23:07

mshockwave force-pushed the dev-sil-diexpr-optimize branch from e9db7d7 to ab126dd Compare August 5, 2021 00:05

adrian-prantl approved these changes Aug 5, 2021

View reviewed changes

mshockwave force-pushed the dev-sil-diexpr-optimize branch from ab126dd to 9a8f2ed Compare August 6, 2021 00:28

mshockwave merged commit c3390ec into swiftlang:main Aug 6, 2021

eeckstein mentioned this pull request Aug 26, 2021

COWOpts: handle debug_value instructions #39059

Merged

mshockwave mentioned this pull request Sep 10, 2021

[DebugInfo] Adding more SIL DIExpression operator and swift::salvageDebugInfo rules #39240

Closed

[SILOptimizer][DebugInfo] Preliminary support for DIExpression in SROA and Mem2Reg #38736

[SILOptimizer][DebugInfo] Preliminary support for DIExpression in SROA and Mem2Reg #38736

Uh oh!

Conversation

mshockwave commented Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mshockwave commented Aug 3, 2021

Uh oh!

swift-ci commented Aug 3, 2021

Uh oh!

Uh oh!

Uh oh!

adrian-prantl Aug 3, 2021

Choose a reason for hiding this comment

Uh oh!

mshockwave Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

adrian-prantl Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

mshockwave Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swift-ci commented Aug 3, 2021

Uh oh!

mshockwave commented Aug 4, 2021

Uh oh!

mshockwave commented Aug 4, 2021

Uh oh!

mshockwave commented Aug 4, 2021

Uh oh!

swift-ci commented Aug 4, 2021

Uh oh!

swift-ci commented Aug 5, 2021

Uh oh!

mshockwave commented Aug 5, 2021

Uh oh!

swift-ci commented Aug 5, 2021

Uh oh!

adrian-prantl left a comment

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Aug 5, 2021

Uh oh!

swift-ci commented Aug 5, 2021

Uh oh!

mshockwave commented Aug 6, 2021

Uh oh!

adrian-prantl commented Aug 6, 2021

Uh oh!

Uh oh!

mshockwave commented Aug 3, 2021 •

edited

Loading