On Jul 17, 2013, at 2:12 AM, Chandler Carruth <chandlerc at google.com> wrote:> On Tue, Jul 16, 2013 at 9:34 PM, Bob Wilson <bob.wilson at apple.com> wrote: > > On Jul 16, 2013, at 5:51 PM, Eli Friedman <eli.friedman at gmail.com> wrote: > >> On Tue, Jul 16, 2013 at 5:21 PM, Quentin Colombet <qcolombet at apple.com> wrote: >>> ** Advices Needed ** >>> >>> 1. Decide whether or not we want such capabilities (if we do not we may just >>> add sporadically the support for a new warning/group of warning/error). >>> 2. Come up with a plan to implement that (assuming we want it). >> >> The frontend should be presenting warnings, not the backend; adding a >> hook which provides the appropriate information shouldn't be too hard. >> Warnings coming out of the backend are very difficult to design well, >> so I don't expect we will add many. Also, keep in mind that the >> information coming out of the backend could be used in other ways; it >> might not make sense for the backend to decide that some piece of >> information should be presented as a warning. (Consider, for example, >> IDE integration to provide additional information about functions and >> loops on demand.) > > I think we definitely need this. In fact, I tried adding something simple earlier this year but gave up when I realized that the task was bigger than I expected. We already have a hook for diagnostics that can be easily extended to handle warnings as well as errors (which is what I tried earlier), but the problem is that it is hardwired for inline assembly errors. To do this right, new warnings really need to be associated with warning groups so that can be controlled from the front-end. > > I agree with Eli that there probably won’t be too many of these. Adding a few new entries to clang’s diagnostic .td files would be fine, except that the backend doesn’t see those. It seems like we will need something in llvm that defines a set of “backend diagnostics”, along with a table in the frontend to correlate those with the corresponding clang diagnostics. That seems awkward at best but maybe it’s tolerable as long as there aren’t many of them. > > I actually think this is the wrong approach, and I don't think it's quite what Eli or I am suggestion (of course, Eli may want to clarify, I'm only really clarifying what *I'm* suggesting. > > I think all of the warnings should be in the frontend, using the standard and existing machinery for generating, controlling, and displaying a warning. We already know how to do that well. The difference is that these warnings will need to query the LLVM layer for detailed information through some defined API, and base the warning on this information. This accomplishes two things: > > 1) It ensures the warning machinery is simple, predictable, and integrates cleanly with everything else in Clang. It does so in the best way by simply being the existing machinery. > > 2) It forces us to design reasonable APIs in LLVM to expose to a FE for this information. A consequence of this will be to sort out the layering issues, etc. Another consequence will be a strong chance of finding general purpose APIs in LLVM that can serve many purposes, not just a warning. Consider JITs and other systems that might benefit from having good APIs for querying the size and makeup (at a high level) of a generated function. > > A nice side-effect is that it simplifies the complexity involved for simple warnings -- now it merely is the complexity of exposing the commensurately simple API in LLVM. If instead we go the route of threading a FE interface for *reporting* warnings into LLVM, we have to thread an interface with sufficient power to express many different concepts.I don't understand what you are proposing. First, let me try to clarify my proposal, in case there was any confusion about that. LLVMContext already has a hook for diagnostics, setInlineAsmDiagnosticHandler() et al. I was suggesting that we rename those interfaces to be more generic, add a simple enumeration of whatever diagnostics can be produced from the backend, and add support in clang for mapping those enumeration values to the corresponding clang diagnostics. This would be a small amount of work and would also be consistent with everything you wrote above about reusing the standard and existing machinery for diagnostics in clang. For the record, I had started down that path in svn commits 171041 and 171047, but I reverted those changes in 174748 and 174860, since they didn't go far enough to make it work properly and it wasn't clear at the time whether we really needed it. Now let me try to understand what you're suggesting…. You refer several times to having clang query the LLVM layer. Is this to determine whether to emit a diagnostic for some condition? How would this work? Would you have clang insert extra passes to check for various conditions that might require diagnostics? I don't see how else you would do it, since clang's interface to the backend just sets up the PerFunctionPasses, PerModulePasses and CodeGenPasses pass managers and then runs them. Assuming you did add some special passes to check for problems, wouldn't those passes have to duplicate a lot of effort in some cases to find the answers? Take for example the existing warnings in IntrinsicLowering::LowerIntrinsicCall. Those badly need to be cleaned up. Would clang run a special pass to check for intrinsics that are not supported by the target? That pass would need to be implemented as part of clang so that it would have access to clang's diagnostic machinery, but it would also need to know details about what intrinsics are supported by the target. Interesting layering problems there…. Apologies if I'm misinterpreting your proposal. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130717/2c7ca302/attachment.html>
Sent from my iPad On Jul 17, 2013, at 8:53 AM, Bob Wilson <bob.wilson at apple.com> wrote:> > On Jul 17, 2013, at 2:12 AM, Chandler Carruth <chandlerc at google.com> wrote: > >> On Tue, Jul 16, 2013 at 9:34 PM, Bob Wilson <bob.wilson at apple.com> wrote: >>> >>> On Jul 16, 2013, at 5:51 PM, Eli Friedman <eli.friedman at gmail.com> wrote: >>> >>>> On Tue, Jul 16, 2013 at 5:21 PM, Quentin Colombet <qcolombet at apple.com> wrote: >>>>> ** Advices Needed ** >>>>> >>>>> 1. Decide whether or not we want such capabilities (if we do not we may just >>>>> add sporadically the support for a new warning/group of warning/error). >>>>> 2. Come up with a plan to implement that (assuming we want it). >>>> >>>> The frontend should be presenting warnings, not the backend; adding a >>>> hook which provides the appropriate information shouldn't be too hard. >>>> Warnings coming out of the backend are very difficult to design well, >>>> so I don't expect we will add many. Also, keep in mind that the >>>> information coming out of the backend could be used in other ways; it >>>> might not make sense for the backend to decide that some piece of >>>> information should be presented as a warning. (Consider, for example, >>>> IDE integration to provide additional information about functions and >>>> loops on demand.) >>> >>> I think we definitely need this. In fact, I tried adding something simple earlier this year but gave up when I realized that the task was bigger than I expected. We already have a hook for diagnostics that can be easily extended to handle warnings as well as errors (which is what I tried earlier), but the problem is that it is hardwired for inline assembly errors. To do this right, new warnings really need to be associated with warning groups so that can be controlled from the front-end. >>> >>> I agree with Eli that there probably won’t be too many of these. Adding a few new entries to clang’s diagnostic .td files would be fine, except that the backend doesn’t see those. It seems like we will need something in llvm that defines a set of “backend diagnostics”, along with a table in the frontend to correlate those with the corresponding clang diagnostics. That seems awkward at best but maybe it’s tolerable as long as there aren’t many of them. >> >> I actually think this is the wrong approach, and I don't think it's quite what Eli or I am suggestion (of course, Eli may want to clarify, I'm only really clarifying what *I'm* suggesting. >> >> I think all of the warnings should be in the frontend, using the standard and existing machinery for generating, controlling, and displaying a warning. We already know how to do that well. The difference is that these warnings will need to query the LLVM layer for detailed information through some defined API, and base the warning on this information. This accomplishes two things: >> >> 1) It ensures the warning machinery is simple, predictable, and integrates cleanly with everything else in Clang. It does so in the best way by simply being the existing machinery. >> >> 2) It forces us to design reasonable APIs in LLVM to expose to a FE for this information. A consequence of this will be to sort out the layering issues, etc. Another consequence will be a strong chance of finding general purpose APIs in LLVM that can serve many purposes, not just a warning. Consider JITs and other systems that might benefit from having good APIs for querying the size and makeup (at a high level) of a generated function. >> >> A nice side-effect is that it simplifies the complexity involved for simple warnings -- now it merely is the complexity of exposing the commensurately simple API in LLVM. If instead we go the route of threading a FE interface for *reporting* warnings into LLVM, we have to thread an interface with sufficient power to express many different concepts. > > I don't understand what you are proposing. > > First, let me try to clarify my proposal, in case there was any confusion about that. LLVMContext already has a hook for diagnostics, setInlineAsmDiagnosticHandler() et al. I was suggesting that we rename those interfaces to be more generic, add a simple enumeration of whatever diagnostics can be produced from the backend, and add support in clang for mapping those enumeration values to the corresponding clang diagnostics. This would be a small amount of work and would also be consistent with everything you wrote above about reusing the standard and existing machinery for diagnostics in clang. For the record, I had started down that path in svn commits 171041 and 171047, but I reverted those changes in 174748 and 174860, since they didn't go far enough to make it work properly and it wasn't clear at the time whether we really needed it. > > Now let me try to understand what you're suggesting…. You refer several times to having clang query the LLVM layer. Is this to determine whether to emit a diagnostic for some condition? How would this work? Would you have clang insert extra passes to check for various conditions that might require diagnostics? I don't see how else you would do it, since clang's interface to the backend just sets up the PerFunctionPasses, PerModulePasses and CodeGenPasses pass managers and then runs them. Assuming you did add some special passes to check for problems, wouldn't those passes have to duplicate a lot of effort in some cases to find the answers? Take for example the existing warnings in IntrinsicLowering::LowerIntrinsicCall. Those badly need to be cleaned up. Would clang run a special pass to check for intrinsics that are not supported by the target? That pass would need to be implemented as part of clang so that it would have access to clang's diagnostic machinery, but it would also need to know details about what intrinsics are supported by the target. Interesting layering problems there…. Apologies if I'm misinterpreting your proposal.We can't assume clang is the frontend or design a system that only works with clang. There are many systems that use llvm which are not even c compilers. Evan> _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130717/6b445044/attachment.html>
Hi, Thanks all for the insight comments. Let me sum up at a high level what proposals we actually have (sorry if I misinterpreted or missed something, do not hesitate to correct me): 1. Make LLVM defines some APIs that exposes some internal information so that a front-end can use them to build-up some diagnostics. 2. Make LLVM builds up a diagnostic and let a front-end maps this diagnostic to the right warning/error group. In my opinion, both approaches are orthogonal and have different advantages based on what goal we are pursuing. To be more specific, with the first approach, front-end people can come up with new warnings/errors and diagnostics without having to modify the back-end. On the other hand, with the second approach, back-end people can emit new warnings/errors without having to modify the front-end (BTW, it would be nice if we come up at least in clang with a consistent way to pass down options for emitting back-end warning/error when applicable, i.e., without -mllvm but with -W<whatever>). What people are thinking? Thanks again for sharing your thoughts, I appreciate. Cheers, -Quentin On Jul 17, 2013, at 9:38 AM, Evan Cheng <evan.cheng at apple.com> wrote:> > > Sent from my iPad > > On Jul 17, 2013, at 8:53 AM, Bob Wilson <bob.wilson at apple.com> wrote: > >> >> On Jul 17, 2013, at 2:12 AM, Chandler Carruth <chandlerc at google.com> wrote: >> >>> On Tue, Jul 16, 2013 at 9:34 PM, Bob Wilson <bob.wilson at apple.com> wrote: >>> >>> On Jul 16, 2013, at 5:51 PM, Eli Friedman <eli.friedman at gmail.com> wrote: >>> >>>> On Tue, Jul 16, 2013 at 5:21 PM, Quentin Colombet <qcolombet at apple.com> wrote: >>>>> ** Advices Needed ** >>>>> >>>>> 1. Decide whether or not we want such capabilities (if we do not we may just >>>>> add sporadically the support for a new warning/group of warning/error). >>>>> 2. Come up with a plan to implement that (assuming we want it). >>>> >>>> The frontend should be presenting warnings, not the backend; adding a >>>> hook which provides the appropriate information shouldn't be too hard. >>>> Warnings coming out of the backend are very difficult to design well, >>>> so I don't expect we will add many. Also, keep in mind that the >>>> information coming out of the backend could be used in other ways; it >>>> might not make sense for the backend to decide that some piece of >>>> information should be presented as a warning. (Consider, for example, >>>> IDE integration to provide additional information about functions and >>>> loops on demand.) >>> >>> I think we definitely need this. In fact, I tried adding something simple earlier this year but gave up when I realized that the task was bigger than I expected. We already have a hook for diagnostics that can be easily extended to handle warnings as well as errors (which is what I tried earlier), but the problem is that it is hardwired for inline assembly errors. To do this right, new warnings really need to be associated with warning groups so that can be controlled from the front-end. >>> >>> I agree with Eli that there probably won’t be too many of these. Adding a few new entries to clang’s diagnostic .td files would be fine, except that the backend doesn’t see those. It seems like we will need something in llvm that defines a set of “backend diagnostics”, along with a table in the frontend to correlate those with the corresponding clang diagnostics. That seems awkward at best but maybe it’s tolerable as long as there aren’t many of them. >>> >>> I actually think this is the wrong approach, and I don't think it's quite what Eli or I am suggestion (of course, Eli may want to clarify, I'm only really clarifying what *I'm* suggesting. >>> >>> I think all of the warnings should be in the frontend, using the standard and existing machinery for generating, controlling, and displaying a warning. We already know how to do that well. The difference is that these warnings will need to query the LLVM layer for detailed information through some defined API, and base the warning on this information. This accomplishes two things: >>> >>> 1) It ensures the warning machinery is simple, predictable, and integrates cleanly with everything else in Clang. It does so in the best way by simply being the existing machinery. >>> >>> 2) It forces us to design reasonable APIs in LLVM to expose to a FE for this information. A consequence of this will be to sort out the layering issues, etc. Another consequence will be a strong chance of finding general purpose APIs in LLVM that can serve many purposes, not just a warning. Consider JITs and other systems that might benefit from having good APIs for querying the size and makeup (at a high level) of a generated function. >>> >>> A nice side-effect is that it simplifies the complexity involved for simple warnings -- now it merely is the complexity of exposing the commensurately simple API in LLVM. If instead we go the route of threading a FE interface for *reporting* warnings into LLVM, we have to thread an interface with sufficient power to express many different concepts. >> >> I don't understand what you are proposing. >> >> First, let me try to clarify my proposal, in case there was any confusion about that. LLVMContext already has a hook for diagnostics, setInlineAsmDiagnosticHandler() et al. I was suggesting that we rename those interfaces to be more generic, add a simple enumeration of whatever diagnostics can be produced from the backend, and add support in clang for mapping those enumeration values to the corresponding clang diagnostics. This would be a small amount of work and would also be consistent with everything you wrote above about reusing the standard and existing machinery for diagnostics in clang. For the record, I had started down that path in svn commits 171041 and 171047, but I reverted those changes in 174748 and 174860, since they didn't go far enough to make it work properly and it wasn't clear at the time whether we really needed it. >> >> Now let me try to understand what you're suggesting…. You refer several times to having clang query the LLVM layer. Is this to determine whether to emit a diagnostic for some condition? How would this work? Would you have clang insert extra passes to check for various conditions that might require diagnostics? I don't see how else you would do it, since clang's interface to the backend just sets up the PerFunctionPasses, PerModulePasses and CodeGenPasses pass managers and then runs them. Assuming you did add some special passes to check for problems, wouldn't those passes have to duplicate a lot of effort in some cases to find the answers? Take for example the existing warnings in IntrinsicLowering::LowerIntrinsicCall. Those badly need to be cleaned up. Would clang run a special pass to check for intrinsics that are not supported by the target? That pass would need to be implemented as part of clang so that it would have access to clang's diagnostic machinery, but it would also need to know details about what intrinsics are supported by the target. Interesting layering problems there…. Apologies if I'm misinterpreting your proposal. > > We can't assume clang is the frontend or design a system that only works with clang. There are many systems that use llvm which are not even c compilers. > > Evan > >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130717/d4b7a3b3/attachment.html>
Sorry, just getting caught up on an old thread. I haven't been involved in discussions of this. On Jul 17, 2013, at 8:53 AM, Bob Wilson <bob.wilson at apple.com> wrote:> First, let me try to clarify my proposal, in case there was any confusion about that. LLVMContext already has a hook for diagnostics, setInlineAsmDiagnosticHandler() et al. I was suggesting that we rename those interfaces to be more generic, add a simple enumeration of whatever diagnostics can be produced from the backend, and add support in clang for mapping those enumeration values to the corresponding clang diagnostics. This would be a small amount of work and would also be consistent with everything you wrote above about reusing the standard and existing machinery for diagnostics in clang.Of all of the proposals discussed, I like this the best: 1) This is a really simple extension of what we already have. 2) The backend providing a set of enumerations for the classes of diagnostics it produces doesn't tie it to clang, and doesn't make it language specific. Clients should be able to completely ignore the enum if they want the current (unclassified) behavior, and if an unknown enum value comes through, it is easy to handle. 3) I don't see how something like the stack size diagnostic can be implemented by clang calling into the backend. First, the MachineFunction (and thus, MachineFrameInfo) is a transient datastructure used by the backend when a function is compiled. There is nothing persistent for clang to query. Second, clang would have to know about all of the LLVM IR functions generated, which is possible, but impractical to track for things like thunks and other implicitly generated entrypoints. What is the specific concern with this approach? I don't see how this couples the backend to the frontend or causes layering violation problems. -Chris
On Sat, Jul 20, 2013 at 9:15 PM, Chris Lattner <clattner at apple.com> wrote:> Sorry, just getting caught up on an old thread. I haven't been involved in discussions of this. > > On Jul 17, 2013, at 8:53 AM, Bob Wilson <bob.wilson at apple.com> wrote: >> First, let me try to clarify my proposal, in case there was any confusion about that. LLVMContext already has a hook for diagnostics, setInlineAsmDiagnosticHandler() et al. I was suggesting that we rename those interfaces to be more generic, add a simple enumeration of whatever diagnostics can be produced from the backend, and add support in clang for mapping those enumeration values to the corresponding clang diagnostics. This would be a small amount of work and would also be consistent with everything you wrote above about reusing the standard and existing machinery for diagnostics in clang. > > Of all of the proposals discussed, I like this the best: > > 1) This is a really simple extension of what we already have. > > 2) The backend providing a set of enumerations for the classes of diagnostics it produces doesn't tie it to clang, and doesn't make it language specific. Clients should be able to completely ignore the enum if they want the current (unclassified) behavior, and if an unknown enum value comes through, it is easy to handle. > > 3) I don't see how something like the stack size diagnostic can be implemented by clang calling into the backend. First, the MachineFunction (and thus, MachineFrameInfo) is a transient datastructure used by the backend when a function is compiled. There is nothing persistent for clang to query. Second, clang would have to know about all of the LLVM IR functions generated, which is possible, but impractical to track for things like thunks and other implicitly generated entrypoints. > > What is the specific concern with this approach? I don't see how this couples the backend to the frontend or causes layering violation problems. >I've not talked with Chandler about this, but to sketch out the way I'd do it (which is similar): Have the backend vend diagnostics, this can be done either with a set of enums and messages like you mentioned, or just have a message and location struct ala: struct Msg { const char *Message; Location Loc; }; that the consumer of the message can use via a handler. Alternately a handler (and we should have a default handler) can be passed in from the printer of the message (the frontend in the case provided) and it can be called on the error message. Absolutely this should be done via the LLVMContext to deal with the case of parallel function passes. class Handler { void printDiagnostic(const char *Message, Location Loc); }; (Note that I didn't say this was a fleshed out design ;) I think I prefer the latter to the former and we'd just need an "diagnostic callback handler" on the context. Though we would need to keep a set of diagnostics that the backend handles. That said, that it provides diagnostics based on its input language seems to make the most sense. It can use the location metadata if it has it to produce a location, otherwise you get the function, etc. in some sort of nicely degraded quality. I think this scheme could also work as a way of dealing with the "Optimization Diary" sort of use that Hal is envisioning as well. Keeping the separation of concerns around where the front end handles diagnostics on what we'll call "source locations" is pretty important, however, I agree that not every warning can be expressed this way, i.e. the stack size diagnostic. However, leaving the printing up to the front end is the best way to deal with this and a generic diagnostic engine would probably help for things like llc/opt where the backend just deals with its input language - IR. The existing inline asm diagnostics are ... problematic and it would definitely be nice to get a generic interface for them. Though they're actually separated into two separate cases where, I think, we end up with our confusion: a) Front end diagnostics - This is an area that needs some work to be decent, but it involves the front end querying the back end for things like register size, valid immediates, etc and should be implemented by the front end with an expanded set of target queries. We could use this as a way to solidify the backend api for the MS inline asm support as well and use some of that when parsing GNU style inline asm. b) Back end diagnostics - This is the stuff that the front end has no hope of diagnosing. i.e. "ran out of registers", or "can't figure out how to split this up into this kind of vector register". The latter has always been a bit funny and I'm always unhappy with it, but I don't have any better ideas. A unified scheme of communicating "help help I'm being oppressed by the front end" in the backend would be, at the very least, a step forward. Thoughts? -eric