Page MenuHomePhabricator

[Polly] Print executed statement instances at runtime.

Authored by Meinersbur on Apr 17 2018, 3:57 PM.



Add the option -polly-codegen-trace-stmts and -polly-codegen-print scalars. When enabled, adds a print to the beginning of every generated statement that prints the executed statement instance. With -polly-codegen-print scalars, it also prints the value of all scalars that are used in the statement, and PHIs defined in the beginning of the statement.

Diff Detail


Event Timeline

Meinersbur created this revision.Apr 17 2018, 3:57 PM

Hi Michael,

thanks for the patch. Looks great!

I have one interface comment, which you might to have a look at.

Otherwise, just minor comments / documentation things. While the original code is not as extensively documented, if it is low overhead you might think of completing some more.

339 ↗(On Diff #142854)


35 ↗(On Diff #142854)

Should we document the arguments?

39 ↗(On Diff #142854)

Maybe add a comment?

76 ↗(On Diff #142854)

Should we document the arguments?

IMPORTANT: I don't think we should expose this function. This is the internal interface, see below.
93 ↗(On Diff #142854)


110 ↗(On Diff #142854)
IMPORTANT: I feel this function needs fixing instead of exposing the function pointed out above. It seems this function does not seem to work and apparently is not even tested, which is why you maybe exposed the other function.

An easy fix could be to change this function to.

Values.insert(Values.begin(), Array.begin(), Array.end());
createPrinter(Builder, UseGPU, Values, args...);

This changes the semantics of this function, in that the original idea was to add whitespaces between elements passed in a vector and the new variant would not do this any more. We could probably add/fix white space insertions if this is needed/useful. I don't have an opinion here, but we should likely document this slightly more explicit.

61 ↗(On Diff #142854)

that _print_

682 ↗(On Diff #142854)

Add a notice to the assert?

692 ↗(On Diff #142854)

I assume you intended to leave this in. That's fine with me, just pointing it out.

727 ↗(On Diff #142854)

do cannot

1494 ↗(On Diff #142854)

Remove #if 1?

As discussed in the phone call, I added missing functionality in 330292.

philip.pfaffe added inline comments.Apr 19 2018, 9:22 AM
36 ↗(On Diff #142854)

Why AddressSpace=4?

Meinersbur added inline comments.Apr 19 2018, 3:21 PM
36 ↗(On Diff #142854)

I don't know, this is refactored from the existing RuntimeDebugBuilderCode, and is the reason why I refactored it out instead of calling CreateGlobalStringPtr directly.

I assume this puts string constants into the .rodata segment, but I didn't care enough to look it up.

grosser added inline comments.Apr 20 2018, 12:43 AM
36 ↗(On Diff #142854)

This is the CUDA constant address space:

Probably comes historically when used for CUDA debugging. It also seems to work for CPUs and is used as an indicator to separate between strings and pointers (strings are printed as strings, pointers as integers).

As this is just a refactoring, we should likely document that we use address-space 4 as indicator for strings (rather than integers). It seems there is no easy way to do this differentiation, but if there is one one we should improve this in a followup patch.

philip.pfaffe added inline comments.Apr 20 2018, 3:37 AM
36 ↗(On Diff #142854)

Do you have a reference for this? I've never seen addrspace used in anything but GPU-Code. If it works, I'd like to be convinced that that's not by chance.

grosser added inline comments.Apr 20 2018, 5:48 AM
36 ↗(On Diff #142854)

The behavior of LLVM for non-zero address spaces is target specific (see langref). It seems the X86 backend does not really consider address space numbers and maps all to address space zero. So yes, it works by-chance to some extend -- meaning it works on X86.

We likely should improve this at some point. Unfortunately there is no easy way to indicate strings. A more conservative approach would be to look through the value and only identify globals which are constant and null-terminated as strings and print everything else as pointers. Maybe that would be good enough -- and would not require address space magic.

philip.pfaffe requested changes to this revision.Apr 20 2018, 6:19 AM
philip.pfaffe added inline comments.
36 ↗(On Diff #142854)

Then this should be fixed right now.

This revision now requires changes to proceed.Apr 20 2018, 6:19 AM
bollu added inline comments.Apr 20 2018, 8:46 AM
36 ↗(On Diff #142854)

I am confused, what is a "reasonable" fix for this? Isn't this _clearly_ target dependent?

philip.pfaffe added inline comments.Apr 25 2018, 10:40 AM
36 ↗(On Diff #142854)

It's _clearly_ target dependent, but it's used as if it weren't. I.e., addrspace 4 ends up on the CPU side of things.

Moreover, I realized that addrspace 4 is even used further down, to identify string constants to guess the right format string option. Which needs to be fixed. I'm happy to do this in a seperate change though, so I think this diff can continue as is (remaining comments aside).

39 ↗(On Diff #142854)

A comment is needed, but why is this on the public API? When am I supposed to use this?

This revision was not accepted when it landed; it landed in state Needs Revision.Apr 25 2018, 12:47 PM
This revision was automatically updated to reflect the committed changes.
Meinersbur marked 11 inline comments as done.