Page MenuHomePhabricator

[Interpreter] Add newline to interpreter debugging output
ClosedPublic

Authored by abrown on Feb 5 2019, 8:25 PM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

abrown created this revision.Feb 5 2019, 8:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 8:25 PM

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

@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.

kristof.beyls accepted this revision.Feb 27 2019, 11:35 PM

LGTM, thanks for fixing this!

This revision is now accepted and ready to land.Feb 27 2019, 11:35 PM
abrown added a comment.Mar 1 2019, 7:16 PM

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

Closed by commit rL355587: Add newline to interpreter debugging output (authored by kbeyls, committed by ). · Explain WhyMar 7 2019, 2:14 AM
This revision was automatically updated to reflect the committed changes.

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.