This is an archive of the discontinued LLVM Phabricator instance.

[DEBUGINFO] Disable unsupported debug info options for NVPTX target.
ClosedPublic

Authored by ABataev on Jul 10 2018, 12:16 PM.

Details

Summary

Some targets support only default set of the debug options and do not
support additional debug options, like NVPTX target. Patch introduced
virtual function supportsNonDefaultDebugOptions() that can be overloaded
by the toolchain, checks if the target supports some additional debug
options and emits warning when an unsupported debug option is
found.

Diff Detail

Event Timeline

ABataev created this revision.Jul 10 2018, 12:16 PM
echristo added a comment.EditedJul 17 2018, 3:14 PM

I think you should break it out on an option by option basis. Just warning on "non-standard" options won't make as much sense to end users. Perhaps a "this option is unsupported on the target you're compiling for" etc.

Thoughts?

I think you should break it out on an option by option basis. Just warning on "non-standard" options won't make as much sense to end users. Perhaps a "this option is unsupported on the target you're compiling for" etc.

Thoughts?

  1. I can split it, no problems.
  2. Hmm, actually this what the warning already says. If the option is not supported it says 'debug option '-xxx' is not supported for target 'xxx-xxx-xxx''. It does not seem to me like a warning on non-standard option.

I think you should break it out on an option by option basis. Just warning on "non-standard" options won't make as much sense to end users. Perhaps a "this option is unsupported on the target you're compiling for" etc.

Thoughts?

  1. I can split it, no problems.
  2. Hmm, actually this what the warning already says. If the option is not supported it says 'debug option '-xxx' is not supported for target 'xxx-xxx-xxx''. It does not seem to me like a warning on non-standard option.

Let me try to elaborate a bit, I agree that I'm not very clear above.

I'm not a fan of the generic "non default debug options". It feels misleading. I think we probably want to separate it by "dwarf extensions", and "dwarf version".

As far as the error message itself: "debug option" sounds like an option to debug clang rather than a debug information option. Perhaps say "debug information option" rather than "debug option"?

I think you should break it out on an option by option basis. Just warning on "non-standard" options won't make as much sense to end users. Perhaps a "this option is unsupported on the target you're compiling for" etc.

Thoughts?

  1. I can split it, no problems.
  2. Hmm, actually this what the warning already says. If the option is not supported it says 'debug option '-xxx' is not supported for target 'xxx-xxx-xxx''. It does not seem to me like a warning on non-standard option.

Let me try to elaborate a bit, I agree that I'm not very clear above.

I'm not a fan of the generic "non default debug options". It feels misleading. I think we probably want to separate it by "dwarf extensions", and "dwarf version".

As far as the error message itself: "debug option" sounds like an option to debug clang rather than a debug information option. Perhaps say "debug information option" rather than "debug option"?

Summarizing offline (irc) conversation:

Let's keep the generic solution that Alexey was using here, but instead migrate it to a "supports debug flag" routine that uses enums to check for which things we support. This way targets that don't quite support various levels of debug info or extensions can define them for themselves and even check OS versions.

(This would probably have been useful for darwin in the past and still might be).

ABataev updated this revision to Diff 156139.Jul 18 2018, 1:10 PM

Renamed function supportsNonDefaultDebugOptions() -> supportsDebugInfoOption(const Arg*). Instead of the enum it accepts and should check one of the debug info options.

Getting close, one inline comment.

lib/Driver/ToolChains/Cuda.h
161

I'd like to see this for all of the possible options (returning false by default is fine), with the ones supported spelled out (-gdwarf/-gdwarf-2/etc). This means you'll probably have to check this at all option processing sites for debug info. Alternately you could check g_group flags as a whole first and then process individually.

ABataev updated this revision to Diff 156575.Jul 20 2018, 1:04 PM

Added checks for all possible debug options.

echristo added inline comments.Jul 25 2018, 1:40 PM
lib/Driver/ToolChains/Clang.cpp
929–934

I'd probably simplify this as:

if (TC.supportsDebugInfoOption(A))
  return true;
reportUnsupportedDebugInfoOption(A, Args, D, TC.getTriple());
return false;

And just leave the action part in the rest of the code. I think that means you could also leave the bool off.

As another optimization here you could, instead, just check all of the arguments ahead of time by getting the full list of debug info and just checking them all. That's probably not worth it though TBH.

ABataev updated this revision to Diff 157563.Jul 26 2018, 1:29 PM

Address ERic's comments.

ABataev marked an inline comment as done.Jul 26 2018, 1:30 PM
ABataev added inline comments.
lib/Driver/ToolChains/Clang.cpp
929–934

Done, but I need boolean result in most cases.

ABataev marked an inline comment as done.Jul 27 2018, 6:23 AM

Eric accepted the patch offline.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 27 2018, 12:46 PM
This revision was automatically updated to reflect the committed changes.
tra added a subscriber: tra.Aug 1 2018, 2:37 PM

I wonder, what's the right thing to do to silence the warnings. For instance, we compile everything with -Werror and the warnings result in build breaks.

Easy way out is to pass -Wno-unsupported-target-opt. It works, but it does not really solve anything. It also may mask potential other problems.

Another alternative is to change clang driver and filter out unsupported options so they are not passed to cc1. That will also work, but it looks wrong, because now we have two patches that effectively cancel each other for no observable benefit.

Third option is to grow a better way to specify target-specific sub-compilation options and then consider fancy debug flags to be attributable to host compilation only. Anything beyond the "safe" subset, would have to be specified explicitly. This also sounds awkward -- I don't really want to replicate bunch of options times number of GPUs I'm compiling for. That may be alleviated by providing more coarse way to group options. E.g. we could say "these are the options for *all* non-host compilations, and here are few specifically for sm_XY". I think @echristo and I had discussed something like this long time ago.

Any other ideas?

In D49148#1184957, @tra wrote:

I wonder, what's the right thing to do to silence the warnings. For instance, we compile everything with -Werror and the warnings result in build breaks.

Easy way out is to pass -Wno-unsupported-target-opt. It works, but it does not really solve anything. It also may mask potential other problems.

Another alternative is to change clang driver and filter out unsupported options so they are not passed to cc1. That will also work, but it looks wrong, because now we have two patches that effectively cancel each other for no observable benefit.

Third option is to grow a better way to specify target-specific sub-compilation options and then consider fancy debug flags to be attributable to host compilation only. Anything beyond the "safe" subset, would have to be specified explicitly. This also sounds awkward -- I don't really want to replicate bunch of options times number of GPUs I'm compiling for. That may be alleviated by providing more coarse way to group options. E.g. we could say "these are the options for *all* non-host compilations, and here are few specifically for sm_XY". I think @echristo and I had discussed something like this long time ago.

I think some amount of #3 is the most reasonable way forward here. Basically the compilation model of "pass the flags and hope they all work on all the targets that we're trying to compile for in this giant multiple target compile" seems to be a long tail of pain :) There are probably things we can do in the clang driver to make this a little easier, e.g. the "all non-host target options" flag you mentioned seems reasonable, but otherwise I think this is up to the user to split out flags that might not work.

Thoughts?

tra added a comment.Aug 1 2018, 3:27 PM

We normally do not need to deviate from the host options all that often. I would argue that keeping options identical is a reasonable default for most options.
For some options the driver may be able to derive a sensible value based on the host options. E.g. some options can be ignored. Some can be downgraded. Some can be replaced with a target-specific equivalent.
For others we must require the user to provide the value.

So, at the very least we must be able to put all options into one of the categories.

We also need to figure out what kind of syntax we'll use to specify target-specific options. We currently have a -Xarch... hack in some toolchains, but it's rather awkward to us in practice as it does not give you much control over where are those options placed on the cc1's command line, they are also rather verbose and usually do not support options with arguments. We could make -Xarch=XYZ a sticky option which would consider following options to apply only to particular arch with, possibly, few special arch names to specify host, device, all subcompilations.

It's also possible that I'm reinventing the wheel here. Are there existing precedents for command-line options with this kind of multi-consumer functionality?