[PATCH] D45743: [Polly] Print executed statement instances at runtime.

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe added inline comments.


================
Comment at: include/polly/CodeGen/RuntimeDebugBuilder.h:36
+                                         llvm::StringRef Str) {
+    return Builder.CreateGlobalStringPtr(Str, "", 4);
+  }
----------------
bollu wrote:
> philip.pfaffe wrote:
> > grosser wrote:
> > > philip.pfaffe wrote:
> > > > grosser wrote:
> > > > > Meinersbur wrote:
> > > > > > philip.pfaffe wrote:
> > > > > > > Why AddressSpace=4?
> > > > > > 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.
> > > > > This is the CUDA constant address space:
> > > > > 
> > > > > https://llvm.org/docs/NVPTXUsage.html#address-spaces
> > > > > 
> > > > > 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.
> > > > 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.
> > > 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.
> > Then this should be fixed right now.
> I am confused, what is a "reasonable" fix for this? Isn't this _clearly_ target dependent?
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).


================
Comment at: include/polly/CodeGen/RuntimeDebugBuilder.h:39
+
+  static bool isPrintable(llvm::Type *Ty);
+
----------------
grosser wrote:
> Maybe add a comment?
A comment is needed, but why is this on the public API? When am I supposed to use this?



Repository:
  rPLO Polly

https://reviews.llvm.org/D45743