[PATCH] D99683: [HIP] Support ThinLTO

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:1904-1907
+def foffload_lto : Flag<["-"], "foffload-lto">, Flags<[CoreOption]>, Group<f_Group>,
+  HelpText<"Enable LTO in 'full' mode for offload compilation">;
+def fno_offload_lto : Flag<["-"], "fno-offload-lto">, Flags<[CoreOption]>, Group<f_Group>,
+  HelpText<"Disable LTO mode (default) for offload compilation">;
----------------
yaxunl wrote:
> jansvoboda11 wrote:
> > yaxunl wrote:
> > > tra wrote:
> > > > Should it be `BoolFOption` ? 
> > > Yes. will fix
> > The `BoolFOption` marshalling multiclass should be only used for flags where either the positive or the negative (or both) are -cc1 options and map to a field in `CompilerInvocation`.
> > 
> > Since this patch only deals with the driver (not the cc1 frontend) using `BoolFOption` is not correct. Please, revert this change to the previous state.
> > 
> > I might need to explicitly call this out in the documentation https://clang.llvm.org/docs/InternalsManual.html#adding-new-command-line-option.
> will do
<off-topic for the patch>

@jansvoboda11 Thank you for the explanation. Updating the docs would indeed be useful.

I would also suggest describing the restrictions next to the `BoolFOption` definition. Developers tend to read the sources way more often than the docs, and the comments source code make it look like a general-purpose multiclass for any boolean `-f` option.

Would it also be possible to add some sort of compile-time safeguards to enforce intended constraints on the CLI tablegen?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99683/new/

https://reviews.llvm.org/D99683