[Interpreter] Add newline to interpreter debugging output

[abrown]

Summary

When running lli --debug --force-interpreter=true the executed instructions are printed but are missing newlines; this patch adds the missing newlines.

[kristof.beyls]

This seems quite obvious.
Typically when we change the behaviour of code, we aim to add a unit/regression test to check that indeed the code now has the intended behaviour (and mostly to make sure future changes do not regress on that expected behaviour).
That being said, this is a change to debug output, for which we typically do not add regression tests.

They only thing that is holding me back to approve this change is that I am completely unfamiliar with the interpreter.
If on this review you could share a small example of behaviour before and after this change as a sanity check, I'd be happy to approve.

Thanks,

Kristof

[abrown]

@kristof.beyls: thanks for taking a look. As expected, without the change we see something like:

...
About to interpret:   store i32 0, i32* %retval, align 4About to interpret:   store i32 %argc, i32* %argc.addr, align 4About to interpret:   store i8** %argv, i8*** %argv.addr, align 8About to interpret:   store i32 10000, i32* %max, align 4About to interpret:   store i32 0, i32* %s, align 4About to interpret:   store i32 2, i32* %n, align 4About to interpret:   br label %while.condAbout to interpret:   %0 = load i32, i32* %n, align 4About to interpret:   %1 = load i32, i32* %max, align 4About to interpret:   %cmp = icmp sle i32 %0, %1About to interpret:   br i1 %cmp, label %while.body, label %while.end10About to interpret:   store i32 1, i32* %p, align 4About to interpret:   store i32 2, i32* %d, align 4About to interpret:   br label %while.cond1About to interpret:   %2 = load i32, i32* %d, align 4About to interpret:   %3 = load i32, i32* %n, align 4About to interpret:   %sub = sub nsw i32 %3, 1About to interpret:   %cmp2 = icmp sle i32 %2, %subAbout to interpret:   br i1 %cmp2, label %while.body3,
...

But with the change the output is a bit more readable:

...
About to interpret:   store i32 %argc, i32* %argc.addr, align 4
About to interpret:   store i8** %argv, i8*** %argv.addr, align 8
About to interpret:   store i32 10000, i32* %max, align 4
About to interpret:   store i32 0, i32* %s, align 4
About to interpret:   store i32 2, i32* %n, align 4
About to interpret:   br label %while.cond
About to interpret:   %0 = load i32, i32* %n, align 4
About to interpret:   %1 = load i32, i32* %max, align 4
About to interpret:   %cmp = icmp sle i32 %0, %1
...

This is likely a non-issue for most people because I would guess few people run this with --force-interpreter=true but the latter output seems more sane.

[greened]

LGTM.

[kristof.beyls]

LGTM, thanks for fixing this!

[abrown]

Thanks for the review. I don't have committer rights; could you merge this for me or point me to how to get access?

[kristof.beyls]

In D57806#1415955, @abrown wrote:

Thanks for the review. I don't have committer rights; could you merge this for me or point me to how to get access?

Hi Andrew,

I just committed this for you. Thanks for the contribution and your patience!
https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access explains how to get commit access.

llvm/trunk/lib/ExecutionEngine/Interpreter/Execution.cpp

void Interpreter::run() {
   while (!ECStack.empty()) {
     // Interpret a single instruction & increment the "PC".
     ExecutionContext &SF = ECStack.back();  // Current stack frame
     Instruction &I = *SF.CurInst++;         // Increment before execute
 
     // Track the number of dynamic instructions executed.
     ++NumDynamicInsts;
 
-    LLVM_DEBUG(dbgs() << "About to interpret: " << I);
+    LLVM_DEBUG(dbgs() << "About to interpret: " << I << "\n");
     visit(I);   // Dispatch to one of the visit* methods...
   }
 }