[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

scott.linder abandoned this revision. scott.linder added a comment. See https://reviews.llvm.org/D56871 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53153/new/ https://reviews.llvm.org/D53153 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

scott.linder added a comment. That sounds reasonable to me. I had already posted a patch with the Driver options, but I will update it to only include the -cc1 version. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53153/new/ https://reviews.llvm.org/D53153 _

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. I don't want this to be a driver option because I don't want to design a general-purpose feature for this right now, nor do I want to gradually accrete a general-purpose feature around a random collection of needs accumulated from other features. Let's just leave it a

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

scott.linder added a comment. I think we need the option to be in the driver, not just -cc1. We are moving towards the clang driver being the authority on how we do offline compilation for AMDGPU, and hiding this in cc1 doesn't help us. I also don't think this is unreasonable to expose in gener

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. It's an interesting idea at least, and it does seem like there ought to be some way to express it, at least internally. A `-cc1` option is fine for your purposes, right? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53153/new/ https://reviews.llvm.org/D53153

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

scott.linder added a comment. It sounds like `-fextern-visiblity=` isn't something we really want, then? I think it would have to be implemented in the abstract linkage computer. Would a patch to add an option to apply the normal visibility calculations to extern decls be reasonable, then? This

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. Mostly, we don't really want the abstract visibility calculation to change whenever we see a definition. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53153/new/ https://reviews.llvm.org/D53153 ___ cfe-commits mai

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

scott.linder added a comment. In D53153#1355718 , @rjmccall wrote: > In D53153#1353256 , @scott.linder > wrote: > > > In D53153#1317977 , @rjmccall > > wrote: > > > > > I t

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. In D53153#1353256 , @scott.linder wrote: > In D53153#1317977 , @rjmccall wrote: > > > I think `-fvisibility=hidden` isn't good enough because you want to infer > > hidden visibility even

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

scott.linder added a comment. In D53153#1317977 , @rjmccall wrote: > I think `-fvisibility=hidden` isn't good enough because you want to infer > hidden visibility even for external symbol references, and that's just not > how global visibility works. Bu

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. I think `-fvisibility=hidden` isn't good enough because you want to infer hidden visibility even for external symbol references, and that's just not how global visibility works. But after this discussion, I'm prepared to accept that (1) we should have some sort of sin

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

scott.linder added a comment. In D53153#1315109 , @rjmccall wrote: > Okay. So it's still the case that all symbols will be defined within the > linkage unit; it's just that some things might need to get exposed outside of > it. > > LLVM does provide a `

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. In D53153#1315107 , @yaxunl wrote: > In D53153#1315039 , @scott.linder > wrote: > > > In D53153#1314798 , @rjmccall > > wrote: > > > > > You still

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

yaxunl added a comment. In D53153#1315039 , @scott.linder wrote: > In D53153#1314798 , @rjmccall wrote: > > > You still have the same linkage model for those other languages, right? > > Ultimately there's somethi

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

scott.linder added a comment. In D53153#1314798 , @rjmccall wrote: > You still have the same linkage model for those other languages, right? > Ultimately there's something like a kernel function that has to be declared > specially and which becomes the

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. In D53153#1314777 , @scott.linder wrote: > In D53153#1291348 , @rjmccall wrote: > > > In D53153#1289380 , @scott.linder > > wrote: > > > > > I don

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

scott.linder added a comment. In D53153#1291348 , @rjmccall wrote: > In D53153#1289380 , @scott.linder > wrote: > > > I don't believe that is currently the case (the unrestricted linking of OCL > > code to OCL cod

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. In https://reviews.llvm.org/D53153#1289380, @scott.linder wrote: > I don't believe that is currently the case (the unrestricted linking of OCL > code to OCL code via a dynamic linker), but we do have the notion of a static > link step, followed by dynamic linking at ru

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

scott.linder added a comment. I don't believe that is currently the case (the unrestricted linking of OCL code to OCL code via a dynamic linker), but we do have the notion of a static link step, followed by dynamic linking at runtime. The static link step is currently via IR, but we plan to sup

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. Okay, that's interesting. And that dynamic linking step includes fairly unrestricted linking of OpenCL code to other OpenCL code, rather than just e.g. loading a single block of OpenCL code that exports a small, fixed interface? If so, then I accept that you need sym

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

t-tye added a comment. In https://reviews.llvm.org/D53153#1288127, @rjmccall wrote: > In https://reviews.llvm.org/D53153#1288112, @rjmccall wrote: > > > But do you want to support *dynamically* linking object files? Because > > that's what visibility is about. > > > To be specific, if you don't

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. In https://reviews.llvm.org/D53153#1288112, @rjmccall wrote: > But do you want to support *dynamically* linking object files? Because > that's what visibility is about. To be specific, if you don't have multiple levels of linking — doing a slower and relatively more

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. In https://reviews.llvm.org/D53153#1288083, @arsenm wrote: > In https://reviews.llvm.org/D53153#1288059, @rjmccall wrote: > > > I agree with Richard that I'm not sure what the point of supporting > > frontend visibility settings in OpenCL is. If you want the "everythin

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

arsenm added a comment. In https://reviews.llvm.org/D53153#1288059, @rjmccall wrote: > I agree with Richard that I'm not sure what the point of supporting frontend > visibility settings in OpenCL is. If you want the "everything is internal to > the image" optimization, presumably you can just

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. I agree with Richard that I'm not sure what the point of supporting frontend visibility settings in OpenCL is. If you want the "everything is internal to the image" optimization, presumably you can just infer visibility on everything in a pass over the IR.