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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f2d72f7
Add source annotation to SILPrinter under flag -sil-print-sourceinfo
meg-gupta Jan 24, 2020
1b9ca41
Add > check to compare previous and current source locations
meg-gupta Jan 27, 2020
d49195c
Add != back. Also print filename:linenumber
meg-gupta Jan 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions include/swift/Basic/SourceManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,9 @@ class SourceManager {
llvm::Optional<unsigned> resolveOffsetForEndOfLine(unsigned BufferId,
unsigned Line) const;

/// Get the length of the line
llvm::Optional<unsigned> getLineLength(unsigned BufferId, unsigned Line) const;

SourceLoc getLocForLineCol(unsigned BufferId, unsigned Line, unsigned Col) const {
auto Offset = resolveFromLineCol(BufferId, Line, Col);
return Offset.hasValue() ? getLocForOffset(BufferId, Offset.getValue()) :
Expand Down
15 changes: 14 additions & 1 deletion lib/Basic/SourceLoc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,10 +331,20 @@ SourceManager::resolveOffsetForEndOfLine(unsigned BufferId,
return resolveFromLineCol(BufferId, Line, ~0u);
}

llvm::Optional<unsigned>
SourceManager::getLineLength(unsigned BufferId, unsigned Line) const {
auto BegOffset = resolveFromLineCol(BufferId, Line, 0);
auto EndOffset = resolveFromLineCol(BufferId, Line, ~0u);
if (BegOffset && EndOffset) {
return EndOffset.getValue() - BegOffset.getValue();
}
return None;
}

llvm::Optional<unsigned> SourceManager::resolveFromLineCol(unsigned BufferId,
unsigned Line,
unsigned Col) const {
if (Line == 0 || Col == 0) {
if (Line == 0) {
return None;
}
const bool LineEnd = Col == ~0u;
Expand All @@ -353,6 +363,9 @@ llvm::Optional<unsigned> SourceManager::resolveFromLineCol(unsigned BufferId,
return None;
}
Ptr = LineStart;
if (Col == 0) {
return Ptr - InputBuf->getBufferStart();
}
// The <= here is to allow for non-inclusive range end positions at EOF
for (; ; ++Ptr) {
--Col;
Expand Down
18 changes: 18 additions & 0 deletions lib/SIL/SILPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ llvm::cl::opt<bool>
SILPrintDebugInfo("sil-print-debuginfo", llvm::cl::init(false),
llvm::cl::desc("Include debug info in SIL output"));

llvm::cl::opt<bool>
SILPrintSourceInfo("sil-print-sourceinfo", llvm::cl::init(false),
llvm::cl::desc("Include source annotation in SIL output"));

llvm::cl::opt<bool> SILPrintGenericSpecializationInfo(
"sil-print-generic-specialization-info", llvm::cl::init(false),
llvm::cl::desc("Include generic specialization"
Expand Down Expand Up @@ -618,7 +622,21 @@ class SILPrinter : public SILInstructionVisitor<SILPrinter> {
}
*this << '\n';

const auto &SM = BB->getModule().getASTContext().SourceMgr;
Optional<SILLocation> PrevLoc;
for (const SILInstruction &I : *BB) {
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.

auto Buffer = SM.findBufferContainingLoc(CurSourceLoc);
auto Line = SM.getLineNumber(CurSourceLoc);
auto LineLength = SM.getLineLength(Buffer, Line);
PrintState.OS << " // " << SM.extractText({SM.getLocForLineCol(Buffer, Line, 0), LineLength.getValueOr(0)}) << "\n";
PrevLoc = I.getLoc();
}
}
}
Ctx.printInstructionCallBack(&I);
if (SILPrintGenericSpecializationInfo) {
if (auto AI = ApplySite::isa(const_cast<SILInstruction *>(&I)))
Expand Down