Skip to content

[Debuginfo] Small fixes #73009

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 16, 2024
Merged

Conversation

Snowy1803
Copy link
Member

This PR includes multiple small fixes, necessary for a bigger change.

  • Fix IRGen inconsistently using the column number
  • Fix SILPrinter printing the debug variable info when debug info printing is disabled
  • Refactor SILCloner to remap the debug variable scope before creating the instruction, instead of after
  • Fix SROA debug variable scope, which could be wrong in some cases (20 occurrences in the stdlib), which brings the number of variables lost by SROA to 0.
  • Call salvage debug info for instructions removed by function passes written in Swift

With those changes, there are 0.06% less variables lost at the SIL level. It does not change the number of variables in the final binary.

@Snowy1803
Copy link
Member Author

@swift-ci please test

Copy link
Contributor

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look a ll good to me, it would be nice to add some tests though, if possible.

The source location for the variable should be the value in VarInfo if set,
otherwise it should use the location of the instruction. Both ways should
be consistent, and as we use column number if VarInfo is set, we have to do
it if isnt, too.
SILBuilderWithScope ignores the scope of the debug value and uses the
scope of the next real instruction. We want to preserve the scope
of the original debug value, so we pass it explicitly.
@Snowy1803 Snowy1803 force-pushed the debuginfo-small-fixes branch 2 times, most recently from 7623a5d to 06d39a2 Compare April 16, 2024 17:54
Salvage Debug Info was only being called for simplifications, and not
for function passes written in Swift. Salvage Debug Info is now called
for both cases.
@Snowy1803 Snowy1803 force-pushed the debuginfo-small-fixes branch from 06d39a2 to 4a543c8 Compare April 16, 2024 17:54
@Snowy1803
Copy link
Member Author

@swift-ci please smoke test

@Snowy1803 Snowy1803 requested a review from adrian-prantl April 16, 2024 17:55
bb0:
%0 = alloc_stack $Int, var, name "a"
%1 = integer_literal $Builtin.Int64, 1
%2 = struct $Int (%1 : $Builtin.Int64)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test will probably not work on 32-bit platforms. Could you rewrite it to hardcode an Int32?

// CHECK: @llvm.dbg.declare(metadata ptr
// CHECK-SAME: metadata ![[VAR_DI:[0-9]+]]
// CHECK-SAME: ), !dbg ![[LOC_DI:[0-9]+]]
debug_value %0 : $Builtin.Int64, var, name "x", loc "simple.swift":1:16, scope 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is fine!

@adrian-prantl adrian-prantl enabled auto-merge April 16, 2024 19:30
@adrian-prantl
Copy link
Contributor

@swift-ci test

@adrian-prantl
Copy link
Contributor

Well, test/SILOptimizer/dead_store_elim.sil seems to pass the bots and does this, so this isn't any worse.

@adrian-prantl adrian-prantl merged commit 6357d12 into swiftlang:main Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants