Skip to content

Add source annotation to SILPrinter under flag -sil-print-sourceinfo #29444

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 3 commits into from
Jan 29, 2020

Conversation

meg-gupta
Copy link
Contributor

This flag will add source annotation in SIL.
rdar://58365252

@meg-gupta
Copy link
Contributor Author

Will look something like this:

bb34:                                             // Preds: bb19
  //     sum += ele.id;
  %364 = class_method %250 : $eletype, #eletype.id!getter.1 : (eletype) -> () -> Int, $@convention(method) (@guaranteed eletype) -> Int // user: %366
  strong_retain %250 : $eletype   

@meg-gupta meg-gupta requested a review from atrick January 24, 2020 23:14
Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

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

Awesome. I have a couple suggestions, but otherwise looks good.

if (SILPrintSourceInfo) {
auto CurSourceLoc = I.getLoc().getSourceLoc();
if (CurSourceLoc.isValid()) {
if (!PrevLoc || SM.getLineNumber(CurSourceLoc) != SM.getLineNumber(PrevLoc->getSourceLoc())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you handle the case where the location distance spans multiple lines? Such as a call with arguments on each line?

I'm not sure if we're currently verifying that locations monotonically increase (@dcci ?), but you may want to check that the current location is greater than the previous location before printing anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated with the > check to compare source locations. And with that a call like:

@inline(never)
func foo() { 
  bar(1, 
  2,
  3)
}

would look like:

// foo()
sil hidden [noinline] @$s9printtest3fooyyF : $@convention(thin) () -> () { 
bb0:
  //   bar(1,
  %0 = integer_literal $Builtin.Int64, 1          // user: %1
  %1 = struct $Int (%0 : $Builtin.Int64)          // user: %7
  //   2,
  %2 = integer_literal $Builtin.Int64, 2          // user: %3
  %3 = struct $Int (%2 : $Builtin.Int64)          // user: %7
  //   3)
  %4 = integer_literal $Builtin.Int64, 3          // user: %5
  %5 = struct $Int (%4 : $Builtin.Int64)          // user: %7
  // function_ref bar(_:_:_:)
  %6 = function_ref @$s9printtest3baryySi_S2itF : $@convention(thin) (Int, Int, Int) -> () // user: %7
  %7 = apply %6(%1, %3, %5) : $@convention(thin) (Int, Int, Int) -> ()
  // } 
  %8 = tuple ()                                   // user: %9
  return %8 : $()                                 // id: %9
} // end sil function '$s9printtest3fooyyF'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a way where I could print all of the call arguments at once looking at the API's available on SourceLoc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't you print all source lines between prevLoc and currLoc? What happens with

func foo(a, b, c) {
  bar(
   a,
   b,
   c)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, for this it prints only the first line:

bb0(%0 : $Int, %1 : $Int, %2 : $Int):
  // func foo(_ a:Int, _ b:Int, _ c:Int) {
  debug_value %0 : $Int, let, name "a", argno 1   // id: %3
  debug_value %1 : $Int, let, name "b", argno 2   // id: %4
  debug_value %2 : $Int, let, name "c", argno 3   // id: %5
  //   bar(a,
  // function_ref bar(_:_:_:)
  %6 = function_ref @$s9printtest3baryySi_S2itF : $@convention(thin) (Int, Int, Int) -> () // user: %7
  %7 = apply %6(%0, %1, %2) : $@convention(thin) (Int, Int, Int) -> () 
  // }  
  %8 = tuple ()                                   // user: %9
  return %8 : $()                                 // id: %9
} // end sil function '$s9printtest3fooyySi_S2itF'

I was worried about hoisting and some other optimizations that can print out too many source lines if I try to print between prevLoc and currLoc.
I think the same issue can happen with checking curLoc > prevLoc now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I misunderstood your intention. If it's more useful to see the source at a fine granularity as the SIL jumps around, then you should go back to the PrevLoc != CurLoc test, but in that case I think you really need to print line numbers to make sense of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I added the line numbers as well now.

Copy link
Member

Choose a reason for hiding this comment

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

@atrick We currently don't have that verification enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dcci Anyway, the verification is for -Onone and it seems that the main use case for this is understanding -O output.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree.

@meg-gupta
Copy link
Contributor Author

@swift-ci please smoke test

@meg-gupta
Copy link
Contributor Author

@swift-ci please smoke test

@meg-gupta meg-gupta merged commit b896013 into swiftlang:master Jan 29, 2020
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.

3 participants