[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdbfa69c5024c: Port some floating point options to new option 
marshalling infrastructure (authored by jansvoboda11, committed by dexonsmith).

Changed prior to commit:
  https://reviews.llvm.org/D82756?vs=301913=303996#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82756

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Option/OptParser.td
  llvm/unittests/Option/CMakeLists.txt
  llvm/unittests/Option/OptionMarshallingTest.cpp
  llvm/unittests/Option/Opts.td
  llvm/utils/TableGen/OptParserEmitter.cpp

Index: llvm/utils/TableGen/OptParserEmitter.cpp
===
--- llvm/utils/TableGen/OptParserEmitter.cpp
+++ llvm/utils/TableGen/OptParserEmitter.cpp
@@ -435,7 +435,12 @@
   OS << "nullptr";
   };
 
-  std::vector> OptsWithMarshalling;
+  auto IsMarshallingOption = [](const Record ) {
+return !isa(R.getValueInit("MarshallingKind")) &&
+   !R.getValueAsString("KeyPath").empty();
+  };
+
+  std::vector OptsWithMarshalling;
   for (unsigned I = 0, E = Opts.size(); I != E; ++I) {
 const Record  = *Opts[I];
 
@@ -443,12 +448,33 @@
 OS << "OPTION(";
 WriteOptRecordFields(OS, R);
 OS << ")\n";
-if (!isa(R.getValueInit("MarshallingKind")))
-  OptsWithMarshalling.push_back(MarshallingKindInfo::create(R));
+if (IsMarshallingOption(R))
+  OptsWithMarshalling.push_back();
   }
   OS << "#endif // OPTION\n";
 
-  for (const auto  : OptsWithMarshalling) {
+  auto CmpMarshallingOpts = [](const Record *const *A, const Record *const *B) {
+unsigned AID = (*A)->getID();
+unsigned BID = (*B)->getID();
+
+if (AID < BID)
+  return -1;
+if (AID > BID)
+  return 1;
+return 0;
+  };
+  // The RecordKeeper stores records (options) in lexicographical order, and we
+  // have reordered the options again when generating prefix groups. We need to
+  // restore the original definition order of options with marshalling to honor
+  // the topology of the dependency graph implied by `DefaultAnyOf`.
+  array_pod_sort(OptsWithMarshalling.begin(), OptsWithMarshalling.end(),
+ CmpMarshallingOpts);
+
+  std::vector> MarshallingKindInfos;
+  for (const auto *R : OptsWithMarshalling)
+MarshallingKindInfos.push_back(MarshallingKindInfo::create(*R));
+
+  for (const auto  : MarshallingKindInfos) {
 OS << "#ifdef " << KindInfo->MacroName << "\n";
 OS << KindInfo->MacroName << "(";
 WriteOptRecordFields(OS, KindInfo->R);
@@ -463,7 +489,7 @@
   OS << "\n";
   OS << MarshallingStringInfo::ValueTablePreamble;
   std::vector ValueTableNames;
-  for (const auto  : OptsWithMarshalling)
+  for (const auto  : MarshallingKindInfos)
 if (auto MaybeValueTableName = KindInfo->emitValueTable(OS))
   ValueTableNames.push_back(*MaybeValueTableName);
 
Index: llvm/unittests/Option/Opts.td
===
--- llvm/unittests/Option/Opts.td
+++ llvm/unittests/Option/Opts.td
@@ -44,3 +44,12 @@
 def Blurmpq_eq : Flag<["--"], "blurmp=">;
 
 def DashDash : Option<["--"], "", KIND_REMAINING_ARGS>;
+
+def marshalled_flag_0 : Flag<["-"], "marshalled-flag-0">,
+  MarshallingInfoFlag<"MarshalledFlag0", DefaultAnyOf<[]>>;
+def marshalled_flag_1 : Flag<["-"], "marshalled-flag-1">,
+  MarshallingInfoFlag<"MarshalledFlag1", DefaultAnyOf<[marshalled_flag_0]>>;
+def marshalled_flag_2 : Flag<["-"], "marshalled-flag-2">,
+  MarshallingInfoFlag<"MarshalledFlag2", DefaultAnyOf<[marshalled_flag_0]>>;
+def marshalled_flag_3 : Flag<["-"], "marshalled-flag-3">,
+  MarshallingInfoFlag<"MarshalledFlag3", DefaultAnyOf<[marshalled_flag_1, marshalled_flag_2]>>;
Index: llvm/unittests/Option/OptionMarshallingTest.cpp
===
--- /dev/null
+++ llvm/unittests/Option/OptionMarshallingTest.cpp
@@ -0,0 +1,47 @@
+//===- unittest/Support/OptionMarshallingTest.cpp - OptParserEmitter tests ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+struct OptionWithMarshallingInfo {
+  const char *Name;
+  const char *KeyPath;
+  const char *DefaultValue;
+};
+
+static const OptionWithMarshallingInfo MarshallingTable[] = {
+#define OPTION_WITH_MARSHALLING_FLAG(PREFIX_TYPE, NAME, ID, KIND, GROUP,   \
+ ALIAS, ALIASARGS, FLAGS, PARAM, HELPTEXT, \
+ 

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D82756#2383317 , @jansvoboda11 
wrote:

> Thanks, `Jan Svoboda` and `jan_svob...@apple.com` is fine. Is it possible to 
> add @dang as a co-author? `git log` says he uses `Daniel Grumberg` and 
> `dany.grumb...@gmail.com`.

I don't think there's a Git feature to support that, but I'll credit Daniel in 
the commit message:

> Port some floating point options to new option marshalling infrastructure
>
> This ports a number of OpenCL and fast-math flags for floating point
> over to the new marshalling infrastructure.
>
> As part of this, `OptInFFlag` was enhanced to allow other flags to imply
> it, via `DefaultAnyOf<>`. For example:
>
>   defm signed_zeros : OptOutFFlag<"signed-zeros", ...,
> "LangOpts->NoSignedZero",
> DefaultAnyOf<[cl_no_signed_zeros, menable_unsafe_fp_math]>>;
>
> defines `-fsigned-zeros` (`false`) and `-fno-signed-zeros` (`true`)
> linked to the keypath `LangOpts->NoSignedZero`, defaulting to `false`,
> but set to `true` implicitly if one of `-cl-no-signed-zeros` or
> `-menable-unsafe-fp-math` is on.
>
> Note that the initial patch was written Daniel Grumberg.

I just rebased and I'm building and running tests now... should get back to 
check on it and commit later today at some point.


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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

Thanks, `Jan Svoboda` and `jan_svob...@apple.com` is fine. Is it possible to 
add @dang as a co-author? `git log` says he uses `Daniel Grumberg` and 
`dany.grumb...@gmail.com`.


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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D82756#2382885 , @jansvoboda11 
wrote:

> @dexonsmith, could you please commit this one for me? I don't have the rights 
> to do so, as this is my first patch.

Sure; what do you want for `GIT_AUTHOR_NAME` and `GIT_AUTHOR_EMAIL`?


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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

@dexonsmith, could you please commit this one for me? I don't have the rights 
to do so, as this is my first patch.


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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese accepted this revision.
Bigcheese added a comment.

LGTM


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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

Thanks for the feedback.

@Bigcheese do you have anything to add?


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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM, thanks for working through this.


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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

I've added tests for OptParserEmitter. Let me know if you had something more 
detailed in mind.




Comment at: clang/include/clang/Driver/Options.td:1176
+defm reciprocal_math : OptInFFlag< "reciprocal-math", "Allow division 
operations to be reassociated", "", "", [], "LangOpts->AllowRecip">;
+def fapprox_func : Flag<["-"], "fapprox-func">, Group, 
Flags<[CC1Option, NoDriverOption]>,
+  MarshallingInfoFlag<"LangOpts->ApproxFunc", "false">;

dexonsmith wrote:
> jansvoboda11 wrote:
> > dexonsmith wrote:
> > > dang wrote:
> > > > Anastasia wrote:
> > > > > could this also be OptInFFlag?
> > > > The aim was to keep the driver semantics the same as before and this 
> > > > was not something you could control with the driver, so I left it as 
> > > > just a CC1 flag. However if it makes sense to be able to control this 
> > > > from the driver then we can definitely make this `OptInFFLag`.
> > > I think adding a driver flag (if that's the right thing to do) should be 
> > > done separately in a follow-up commit.
> > > 
> > > Also for a separate commit: it would be a great improvement if you could 
> > > have OptIn / OptOut flags that were `-cc1`-only (maybe `CC1OptInFFlag`).
> > > - Both `-fX` and `-fno-X` would be parsed by `-cc1` (and cancel each 
> > > other out).
> > > - Only the non-default one would be generated when serializing to `-cc1` 
> > > from `CompilerInvocation` (for `OptIn`, we'd never generate `-fno-X`).
> > > - Neither would be recognized by the driver.
> > > 
> > > I suggest we might want that for most `-cc11` flags. This would make it 
> > > easier to poke through the driver with `-Xclang` to override `-cc1` 
> > > options the driver adds. Not something we want for end-users, but useful 
> > > for debugging the compiler itself. Currently the workflow is to run the 
> > > driver with `-###`, copy/paste, search for and remove the option you want 
> > > to skip, and finally run the `-cc1` command...
> > > 
> > > The reason I bring it up is that it's possible people will start using 
> > > `OptInFFLag` just in order to get this behaviour, not because they intend 
> > > to add a driver flag.
> > I agree that making all `OptInFFlag` and `OptOutFFlag` driver flags as well 
> > as `-cc1` flags by default is not great. How would we go about deciding 
> > which options are okay to demote to `-cc1`-only? Perhaps those not present 
> > in `ClangCommandLineReference.rst` and driver invocations in tests?
> > How would we go about deciding which options are okay to demote to 
> > `-cc1-only`?
> 
> The key is not to add (or remove) driver options unintentionally. Driver 
> options are `clang`'s public interface, and once an option shows up there 
> we're supposed to support it "forever". We shouldn't be 
> accidentally/incidentally growing that surface area in order to simplify 
> parsing/generating `-cc1` command-lines.
> 
> I based my comment on @dang's reason for not using `OptInFFLag`, which I 
> agree with:
> > The aim was to keep the driver semantics the same as before and this was 
> > not something you could control with the driver, so I left it as just a CC1 
> > flag.
> 
Agreed.



Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:468-469
+  // Restore the definition order of marshalling options.
+  array_pod_sort(OptsWithMarshalling.begin(), OptsWithMarshalling.end(),
+ CmpMarshallingOpts);
+

dexonsmith wrote:
> jansvoboda11 wrote:
> > dexonsmith wrote:
> > > I'm curious if this is necessary. If so, how do the options get 
> > > out-of-order?
> > > 
> > > Also, a cleaner way to call `array_pod_sort` would be:
> > > ```
> > > llvm::sort(OptsWithMarshalling, CmpMarshallingOpts);
> > > ```
> > > and I would be tempted to define the lambda inline in the call to 
> > > `llvm::sort`.
> > > 
> > > If it's not necessary, I suggest replacing with an assertion:
> > > ```
> > > assert(llvm::is_sorted(OptsWithMarshalling, ...));
> > > ```
> > 1. The options get out of order during parsing. The `RecordKeeper` stores 
> > records in `std::map, std::less<>>` 
> > that maintains lexicographical order.
> > 
> > 2. Later, they are reordered in this function before prefix groups are 
> > generated: `array_pod_sort(Opts.begin(), Opts.end(), 
> > CompareOptionRecords);`.
> > 
> > 3. Before we generate the marshalling code, we need to restore the 
> > definition order so that we don't use a `LangOpts` or `CodeGenOpts` field 
> > (from `DefaultAnyOf`) before it was initialized.
> > 
> > I've added more detailed explanation to the comment.
> > 
> > I used `array_pod_sort` to be consistent with what's already used here in 
> > `OptParserEmitter.cpp`. I will switch to `llvm::sort` to be more concise if 
> > we don't mind the potential code bloat described here 
> > .
> Thanks for the explanation about the ordering, this makes 

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 301913.
jansvoboda11 added a comment.
Herald added a subscriber: mgorny.

Added LLVM unit tests, reverted back to `array_pod_sort`.


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

https://reviews.llvm.org/D82756

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Option/OptParser.td
  llvm/unittests/Option/CMakeLists.txt
  llvm/unittests/Option/OptionMarshallingTest.cpp
  llvm/unittests/Option/Opts.td
  llvm/utils/TableGen/OptParserEmitter.cpp

Index: llvm/utils/TableGen/OptParserEmitter.cpp
===
--- llvm/utils/TableGen/OptParserEmitter.cpp
+++ llvm/utils/TableGen/OptParserEmitter.cpp
@@ -435,7 +435,12 @@
   OS << "nullptr";
   };
 
-  std::vector> OptsWithMarshalling;
+  auto IsMarshallingOption = [](const Record ) {
+return !isa(R.getValueInit("MarshallingKind")) &&
+   !R.getValueAsString("KeyPath").empty();
+  };
+
+  std::vector OptsWithMarshalling;
   for (unsigned I = 0, E = Opts.size(); I != E; ++I) {
 const Record  = *Opts[I];
 
@@ -443,12 +448,33 @@
 OS << "OPTION(";
 WriteOptRecordFields(OS, R);
 OS << ")\n";
-if (!isa(R.getValueInit("MarshallingKind")))
-  OptsWithMarshalling.push_back(MarshallingKindInfo::create(R));
+if (IsMarshallingOption(R))
+  OptsWithMarshalling.push_back();
   }
   OS << "#endif // OPTION\n";
 
-  for (const auto  : OptsWithMarshalling) {
+  auto CmpMarshallingOpts = [](const Record *const *A, const Record *const *B) {
+unsigned AID = (*A)->getID();
+unsigned BID = (*B)->getID();
+
+if (AID < BID)
+  return -1;
+if (AID > BID)
+  return 1;
+return 0;
+  };
+  // The RecordKeeper stores records (options) in lexicographical order, and we
+  // have reordered the options again when generating prefix groups. We need to
+  // restore the original definition order of options with marshalling to honor
+  // the topology of the dependency graph implied by `DefaultAnyOf`.
+  array_pod_sort(OptsWithMarshalling.begin(), OptsWithMarshalling.end(),
+ CmpMarshallingOpts);
+
+  std::vector> MarshallingKindInfos;
+  for (const auto *R : OptsWithMarshalling)
+MarshallingKindInfos.push_back(MarshallingKindInfo::create(*R));
+
+  for (const auto  : MarshallingKindInfos) {
 OS << "#ifdef " << KindInfo->MacroName << "\n";
 OS << KindInfo->MacroName << "(";
 WriteOptRecordFields(OS, KindInfo->R);
@@ -463,7 +489,7 @@
   OS << "\n";
   OS << MarshallingStringInfo::ValueTablePreamble;
   std::vector ValueTableNames;
-  for (const auto  : OptsWithMarshalling)
+  for (const auto  : MarshallingKindInfos)
 if (auto MaybeValueTableName = KindInfo->emitValueTable(OS))
   ValueTableNames.push_back(*MaybeValueTableName);
 
Index: llvm/unittests/Option/Opts.td
===
--- llvm/unittests/Option/Opts.td
+++ llvm/unittests/Option/Opts.td
@@ -44,3 +44,12 @@
 def Blurmpq_eq : Flag<["--"], "blurmp=">;
 
 def DashDash : Option<["--"], "", KIND_REMAINING_ARGS>;
+
+def marshalled_flag_0 : Flag<["-"], "marshalled-flag-0">,
+  MarshallingInfoFlag<"MarshalledFlag0", DefaultAnyOf<[]>>;
+def marshalled_flag_1 : Flag<["-"], "marshalled-flag-1">,
+  MarshallingInfoFlag<"MarshalledFlag1", DefaultAnyOf<[marshalled_flag_0]>>;
+def marshalled_flag_2 : Flag<["-"], "marshalled-flag-2">,
+  MarshallingInfoFlag<"MarshalledFlag2", DefaultAnyOf<[marshalled_flag_0]>>;
+def marshalled_flag_3 : Flag<["-"], "marshalled-flag-3">,
+  MarshallingInfoFlag<"MarshalledFlag3", DefaultAnyOf<[marshalled_flag_1, marshalled_flag_2]>>;
Index: llvm/unittests/Option/OptionMarshallingTest.cpp
===
--- /dev/null
+++ llvm/unittests/Option/OptionMarshallingTest.cpp
@@ -0,0 +1,47 @@
+//===- unittest/Support/OptionMarshallingTest.cpp - OptParserEmitter tests ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+struct OptionWithMarshallingInfo {
+  const char *Name;
+  const char *KeyPath;
+  const char *DefaultValue;
+};
+
+static const OptionWithMarshallingInfo MarshallingTable[] = {
+#define OPTION_WITH_MARSHALLING_FLAG(PREFIX_TYPE, NAME, ID, KIND, GROUP,   \
+ ALIAS, ALIASARGS, FLAGS, PARAM, HELPTEXT, \
+ METAVAR, VALUES, SPELLING, ALWAYS_EMIT,   \
+ KEYPATH, DEFAULT_VALUE, IS_POSITIVE)  \
+  { NAME, #KEYPATH, #DEFAULT_VALUE },
+#include "Opts.inc"

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D82756#2361565 , @jansvoboda11 
wrote:

> Thanks for the feedback Duncan.
>
> I don't think this patch introduces any changes the parser. We only change 
> the way how  `CodeGenOpts` and `LangOpts` get populated when using 
> `DefaultAnyOf<[...]>`. I've added a test of `CompilerInvocation` that checks 
> just that.

The test for `CompilerInvocation` looks great, but IMO it's insufficient.

Given that the changes are in `llvm/`, it seems best to have a test there so 
that `check-llvm` (also) catches any breakage. I took a look at 
`llvm/unittests/Option/Opts.td` and 
`llvm/unittests/Option/OptionParsingTest.cpp` and I see we don't currently have 
any tests for marshalling, but my intuition is it wouldn't be hard to do. What 
I suggest is adding `OptionMarshallingTest.cpp` and just test the new behaviour 
from this commit (key properties of the changes you made to `OptParser.td` and 
`OptParserEmitter.cpp`), leaving testing the rest for some follow-up.




Comment at: clang/include/clang/Driver/Options.td:1176
+defm reciprocal_math : OptInFFlag< "reciprocal-math", "Allow division 
operations to be reassociated", "", "", [], "LangOpts->AllowRecip">;
+def fapprox_func : Flag<["-"], "fapprox-func">, Group, 
Flags<[CC1Option, NoDriverOption]>,
+  MarshallingInfoFlag<"LangOpts->ApproxFunc", "false">;

jansvoboda11 wrote:
> dexonsmith wrote:
> > dang wrote:
> > > Anastasia wrote:
> > > > could this also be OptInFFlag?
> > > The aim was to keep the driver semantics the same as before and this was 
> > > not something you could control with the driver, so I left it as just a 
> > > CC1 flag. However if it makes sense to be able to control this from the 
> > > driver then we can definitely make this `OptInFFLag`.
> > I think adding a driver flag (if that's the right thing to do) should be 
> > done separately in a follow-up commit.
> > 
> > Also for a separate commit: it would be a great improvement if you could 
> > have OptIn / OptOut flags that were `-cc1`-only (maybe `CC1OptInFFlag`).
> > - Both `-fX` and `-fno-X` would be parsed by `-cc1` (and cancel each other 
> > out).
> > - Only the non-default one would be generated when serializing to `-cc1` 
> > from `CompilerInvocation` (for `OptIn`, we'd never generate `-fno-X`).
> > - Neither would be recognized by the driver.
> > 
> > I suggest we might want that for most `-cc11` flags. This would make it 
> > easier to poke through the driver with `-Xclang` to override `-cc1` options 
> > the driver adds. Not something we want for end-users, but useful for 
> > debugging the compiler itself. Currently the workflow is to run the driver 
> > with `-###`, copy/paste, search for and remove the option you want to skip, 
> > and finally run the `-cc1` command...
> > 
> > The reason I bring it up is that it's possible people will start using 
> > `OptInFFLag` just in order to get this behaviour, not because they intend 
> > to add a driver flag.
> I agree that making all `OptInFFlag` and `OptOutFFlag` driver flags as well 
> as `-cc1` flags by default is not great. How would we go about deciding which 
> options are okay to demote to `-cc1`-only? Perhaps those not present in 
> `ClangCommandLineReference.rst` and driver invocations in tests?
> How would we go about deciding which options are okay to demote to 
> `-cc1-only`?

The key is not to add (or remove) driver options unintentionally. Driver 
options are `clang`'s public interface, and once an option shows up there we're 
supposed to support it "forever". We shouldn't be accidentally/incidentally 
growing that surface area in order to simplify parsing/generating `-cc1` 
command-lines.

I based my comment on @dang's reason for not using `OptInFFLag`, which I agree 
with:
> The aim was to keep the driver semantics the same as before and this was not 
> something you could control with the driver, so I left it as just a CC1 flag.




Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:460-464
+if (AID < BID)
+  return -1;
+if (AID > BID)
+  return 1;
+return 0;

jansvoboda11 wrote:
> dexonsmith wrote:
> > I think `array_pod_sort` will use this like a `bool`, similar to 
> > `std::sort`, in which case you I think you want:
> > ```
> >   return (*A)->getID() < (*B)->getID();
> > ```
> I see that `array_pod_sort` calls `qsort` from the C standard library, which 
> should use the result of comparator as an `int`.
Thanks, you're right, I misremembered `array_pod_sort` somehow reinterpreting 
the lambda...



Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:468-469
+  // Restore the definition order of marshalling options.
+  array_pod_sort(OptsWithMarshalling.begin(), OptsWithMarshalling.end(),
+ CmpMarshallingOpts);
+

jansvoboda11 wrote:
> dexonsmith wrote:
> > I'm curious if this is necessary. If so, how do the 

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 301564.
jansvoboda11 added a comment.
Herald added a subscriber: mgrang.

Integrated code review suggestions.


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

https://reviews.llvm.org/D82756

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Option/OptParser.td
  llvm/utils/TableGen/OptParserEmitter.cpp

Index: llvm/utils/TableGen/OptParserEmitter.cpp
===
--- llvm/utils/TableGen/OptParserEmitter.cpp
+++ llvm/utils/TableGen/OptParserEmitter.cpp
@@ -435,7 +435,12 @@
   OS << "nullptr";
   };
 
-  std::vector> OptsWithMarshalling;
+  auto IsMarshallingOption = [](const Record ) {
+return !isa(R.getValueInit("MarshallingKind")) &&
+   !R.getValueAsString("KeyPath").empty();
+  };
+
+  std::vector OptsWithMarshalling;
   for (unsigned I = 0, E = Opts.size(); I != E; ++I) {
 const Record  = *Opts[I];
 
@@ -443,12 +448,24 @@
 OS << "OPTION(";
 WriteOptRecordFields(OS, R);
 OS << ")\n";
-if (!isa(R.getValueInit("MarshallingKind")))
-  OptsWithMarshalling.push_back(MarshallingKindInfo::create(R));
+if (IsMarshallingOption(R))
+  OptsWithMarshalling.push_back();
   }
   OS << "#endif // OPTION\n";
 
-  for (const auto  : OptsWithMarshalling) {
+  // The RecordKeeper stores records (options) in lexicographical order, and we
+  // have reordered the options again when generating prefix groups. We need to
+  // restore the original definition order of options with marshalling to honor
+  // the topology of the dependency graph implied by `DefaultAnyOf`.
+  llvm::sort(OptsWithMarshalling, [](const Record *A, const Record *B) {
+return A->getID() < B->getID();
+  });
+
+  std::vector> MarshallingKindInfos;
+  for (const auto *R : OptsWithMarshalling)
+MarshallingKindInfos.push_back(MarshallingKindInfo::create(*R));
+
+  for (const auto  : MarshallingKindInfos) {
 OS << "#ifdef " << KindInfo->MacroName << "\n";
 OS << KindInfo->MacroName << "(";
 WriteOptRecordFields(OS, KindInfo->R);
@@ -463,7 +480,7 @@
   OS << "\n";
   OS << MarshallingStringInfo::ValueTablePreamble;
   std::vector ValueTableNames;
-  for (const auto  : OptsWithMarshalling)
+  for (const auto  : MarshallingKindInfos)
 if (auto MaybeValueTableName = KindInfo->emitValueTable(OS))
   ValueTableNames.push_back(*MaybeValueTableName);
 
Index: llvm/include/llvm/Option/OptParser.td
===
--- llvm/include/llvm/Option/OptParser.td
+++ llvm/include/llvm/Option/OptParser.td
@@ -144,6 +144,11 @@
 
 // Helpers for defining marshalling information.
 
+class DefaultAnyOf defaults> {
+  code DefaultValue = !foldl("false", defaults, accumulator, option,
+ !strconcat(accumulator, " || ", !cast(option.KeyPath)));
+}
+
 class MarshallingInfo {
   code KeyPath = keypath;
   code DefaultValue = defaultvalue;
@@ -154,8 +159,8 @@
   code NormalizerRetTy = normalizerretty;
 }
 
-class MarshallingInfoFlag
-  : MarshallingInfo {
+class MarshallingInfoFlag>
+  : MarshallingInfo {
   string MarshallingKind = "flag";
 }
 
Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -37,6 +37,25 @@
   : Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions())) {}
 };
 
+TEST(OptsPopulationTest, CanPopulateOptsWithImpliedFlags) {
+  const char *Args[] = {"clang", "-xc++", "-cl-unsafe-math-optimizations"};
+
+  auto Diags = CompilerInstance::createDiagnostics(new DiagnosticOptions());
+
+  CompilerInvocation CInvok;
+  CompilerInvocation::CreateFromArgs(CInvok, Args, *Diags);
+
+  // Explicitly provided flag.
+  ASSERT_EQ(CInvok.getLangOpts()->CLUnsafeMath, true);
+
+  // Flags directly implied by explicitly provided flag.
+  ASSERT_EQ(CInvok.getCodeGenOpts().LessPreciseFPMAD, true);
+  ASSERT_EQ(CInvok.getLangOpts()->UnsafeFPMath, true);
+
+  // Flag transitively implied by explicitly provided flag.
+  ASSERT_EQ(CInvok.getLangOpts()->AllowRecip, true);
+}
+
 TEST_F(CC1CommandLineGenerationTest, CanGenerateCC1CommandLineFlag) {
   const char *Args[] = {"clang", "-xc++", "-fmodules-strict-context-hash", "-"};
 
@@ -115,4 +134,20 @@
   ASSERT_THAT(GeneratedArgs, Each(StrNe(RelocationModelCStr)));
 }
 
+TEST_F(CC1CommandLineGenerationTest, CanGenerateCC1CommandLineImpliedFlags) {
+  const char *Args[] = {"clang", "-xc++", "-cl-unsafe-math-optimizations",
+"-cl-mad-enable", "-menable-unsafe-fp-math"};
+
+  CompilerInvocation CInvok;
+  CompilerInvocation::CreateFromArgs(CInvok, Args, *Diags);
+
+  

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

Thanks for the feedback Duncan.

I don't think this patch introduces any changes the parser. We only change the 
way how  `CodeGenOpts` and `LangOpts` get populated when using 
`DefaultAnyOf<[...]>`. I've added a test of `CompilerInvocation` that checks 
just that.




Comment at: clang/include/clang/Driver/Options.td:1176
+defm reciprocal_math : OptInFFlag< "reciprocal-math", "Allow division 
operations to be reassociated", "", "", [], "LangOpts->AllowRecip">;
+def fapprox_func : Flag<["-"], "fapprox-func">, Group, 
Flags<[CC1Option, NoDriverOption]>,
+  MarshallingInfoFlag<"LangOpts->ApproxFunc", "false">;

dexonsmith wrote:
> dang wrote:
> > Anastasia wrote:
> > > could this also be OptInFFlag?
> > The aim was to keep the driver semantics the same as before and this was 
> > not something you could control with the driver, so I left it as just a CC1 
> > flag. However if it makes sense to be able to control this from the driver 
> > then we can definitely make this `OptInFFLag`.
> I think adding a driver flag (if that's the right thing to do) should be done 
> separately in a follow-up commit.
> 
> Also for a separate commit: it would be a great improvement if you could have 
> OptIn / OptOut flags that were `-cc1`-only (maybe `CC1OptInFFlag`).
> - Both `-fX` and `-fno-X` would be parsed by `-cc1` (and cancel each other 
> out).
> - Only the non-default one would be generated when serializing to `-cc1` from 
> `CompilerInvocation` (for `OptIn`, we'd never generate `-fno-X`).
> - Neither would be recognized by the driver.
> 
> I suggest we might want that for most `-cc11` flags. This would make it 
> easier to poke through the driver with `-Xclang` to override `-cc1` options 
> the driver adds. Not something we want for end-users, but useful for 
> debugging the compiler itself. Currently the workflow is to run the driver 
> with `-###`, copy/paste, search for and remove the option you want to skip, 
> and finally run the `-cc1` command...
> 
> The reason I bring it up is that it's possible people will start using 
> `OptInFFLag` just in order to get this behaviour, not because they intend to 
> add a driver flag.
I agree that making all `OptInFFlag` and `OptOutFFlag` driver flags as well as 
`-cc1` flags by default is not great. How would we go about deciding which 
options are okay to demote to `-cc1`-only? Perhaps those not present in 
`ClangCommandLineReference.rst` and driver invocations in tests?



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3707
 #undef OPTION_WITH_MARSHALLING_FLAG
+
   return true;

dexonsmith wrote:
> I don't have an opinion about whether there should be a newline here, but 
> please make unrelated whitespace changes like this in a separate commit 
> (before/after).
Got it.



Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:460-464
+if (AID < BID)
+  return -1;
+if (AID > BID)
+  return 1;
+return 0;

dexonsmith wrote:
> I think `array_pod_sort` will use this like a `bool`, similar to `std::sort`, 
> in which case you I think you want:
> ```
>   return (*A)->getID() < (*B)->getID();
> ```
I see that `array_pod_sort` calls `qsort` from the C standard library, which 
should use the result of comparator as an `int`.



Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:468-469
+  // Restore the definition order of marshalling options.
+  array_pod_sort(OptsWithMarshalling.begin(), OptsWithMarshalling.end(),
+ CmpMarshallingOpts);
+

dexonsmith wrote:
> I'm curious if this is necessary. If so, how do the options get out-of-order?
> 
> Also, a cleaner way to call `array_pod_sort` would be:
> ```
> llvm::sort(OptsWithMarshalling, CmpMarshallingOpts);
> ```
> and I would be tempted to define the lambda inline in the call to 
> `llvm::sort`.
> 
> If it's not necessary, I suggest replacing with an assertion:
> ```
> assert(llvm::is_sorted(OptsWithMarshalling, ...));
> ```
1. The options get out of order during parsing. The `RecordKeeper` stores 
records in `std::map, std::less<>>` that 
maintains lexicographical order.

2. Later, they are reordered in this function before prefix groups are 
generated: `array_pod_sort(Opts.begin(), Opts.end(), CompareOptionRecords);`.

3. Before we generate the marshalling code, we need to restore the definition 
order so that we don't use a `LangOpts` or `CodeGenOpts` field (from 
`DefaultAnyOf`) before it was initialized.

I've added more detailed explanation to the comment.

I used `array_pod_sort` to be consistent with what's already used here in 
`OptParserEmitter.cpp`. I will switch to `llvm::sort` to be more concise if we 
don't mind the potential code bloat described here 
.


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

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

I like how this is coming together. I have a few comments inline.

Also, I wonder if there should be a test for the new OptParser behaviour in 
`llvm/unittests/Option/`.




Comment at: clang/include/clang/Driver/Options.td:1176
+defm reciprocal_math : OptInFFlag< "reciprocal-math", "Allow division 
operations to be reassociated", "", "", [], "LangOpts->AllowRecip">;
+def fapprox_func : Flag<["-"], "fapprox-func">, Group, 
Flags<[CC1Option, NoDriverOption]>,
+  MarshallingInfoFlag<"LangOpts->ApproxFunc", "false">;

dang wrote:
> Anastasia wrote:
> > could this also be OptInFFlag?
> The aim was to keep the driver semantics the same as before and this was not 
> something you could control with the driver, so I left it as just a CC1 flag. 
> However if it makes sense to be able to control this from the driver then we 
> can definitely make this `OptInFFLag`.
I think adding a driver flag (if that's the right thing to do) should be done 
separately in a follow-up commit.

Also for a separate commit: it would be a great improvement if you could have 
OptIn / OptOut flags that were `-cc1`-only (maybe `CC1OptInFFlag`).
- Both `-fX` and `-fno-X` would be parsed by `-cc1` (and cancel each other out).
- Only the non-default one would be generated when serializing to `-cc1` from 
`CompilerInvocation` (for `OptIn`, we'd never generate `-fno-X`).
- Neither would be recognized by the driver.

I suggest we might want that for most `-cc11` flags. This would make it easier 
to poke through the driver with `-Xclang` to override `-cc1` options the driver 
adds. Not something we want for end-users, but useful for debugging the 
compiler itself. Currently the workflow is to run the driver with `-###`, 
copy/paste, search for and remove the option you want to skip, and finally run 
the `-cc1` command...

The reason I bring it up is that it's possible people will start using 
`OptInFFLag` just in order to get this behaviour, not because they intend to 
add a driver flag.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3707
 #undef OPTION_WITH_MARSHALLING_FLAG
+
   return true;

I don't have an opinion about whether there should be a newline here, but 
please make unrelated whitespace changes like this in a separate commit 
(before/after).



Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:460-464
+if (AID < BID)
+  return -1;
+if (AID > BID)
+  return 1;
+return 0;

I think `array_pod_sort` will use this like a `bool`, similar to `std::sort`, 
in which case you I think you want:
```
  return (*A)->getID() < (*B)->getID();
```



Comment at: llvm/utils/TableGen/OptParserEmitter.cpp:468-469
+  // Restore the definition order of marshalling options.
+  array_pod_sort(OptsWithMarshalling.begin(), OptsWithMarshalling.end(),
+ CmpMarshallingOpts);
+

I'm curious if this is necessary. If so, how do the options get out-of-order?

Also, a cleaner way to call `array_pod_sort` would be:
```
llvm::sort(OptsWithMarshalling, CmpMarshallingOpts);
```
and I would be tempted to define the lambda inline in the call to `llvm::sort`.

If it's not necessary, I suggest replacing with an assertion:
```
assert(llvm::is_sorted(OptsWithMarshalling, ...));
```


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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 300979.
jansvoboda11 added a comment.

Fix typo & whitespace.


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

https://reviews.llvm.org/D82756

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Option/OptParser.td
  llvm/utils/TableGen/OptParserEmitter.cpp

Index: llvm/utils/TableGen/OptParserEmitter.cpp
===
--- llvm/utils/TableGen/OptParserEmitter.cpp
+++ llvm/utils/TableGen/OptParserEmitter.cpp
@@ -435,7 +435,12 @@
   OS << "nullptr";
   };
 
-  std::vector> OptsWithMarshalling;
+  auto IsMarshallingOption = [](const Record ) {
+return !isa(R.getValueInit("MarshallingKind")) &&
+   !R.getValueAsString("KeyPath").empty();
+  };
+
+  std::vector OptsWithMarshalling;
   for (unsigned I = 0, E = Opts.size(); I != E; ++I) {
 const Record  = *Opts[I];
 
@@ -443,12 +448,31 @@
 OS << "OPTION(";
 WriteOptRecordFields(OS, R);
 OS << ")\n";
-if (!isa(R.getValueInit("MarshallingKind")))
-  OptsWithMarshalling.push_back(MarshallingKindInfo::create(R));
+if (IsMarshallingOption(R))
+  OptsWithMarshalling.push_back();
   }
   OS << "#endif // OPTION\n";
 
-  for (const auto  : OptsWithMarshalling) {
+  auto CmpMarshallingOpts = [](const Record *const *A, const Record *const *B) {
+unsigned AID = (*A)->getID();
+unsigned BID = (*B)->getID();
+
+if (AID < BID)
+  return -1;
+if (AID > BID)
+  return 1;
+return 0;
+  };
+
+  // Restore the definition order of marshalling options.
+  array_pod_sort(OptsWithMarshalling.begin(), OptsWithMarshalling.end(),
+ CmpMarshallingOpts);
+
+  std::vector> MarshallingKindInfos;
+  for (const auto *R : OptsWithMarshalling)
+MarshallingKindInfos.push_back(MarshallingKindInfo::create(*R));
+
+  for (const auto  : MarshallingKindInfos) {
 OS << "#ifdef " << KindInfo->MacroName << "\n";
 OS << KindInfo->MacroName << "(";
 WriteOptRecordFields(OS, KindInfo->R);
@@ -463,7 +487,7 @@
   OS << "\n";
   OS << MarshallingStringInfo::ValueTablePreamble;
   std::vector ValueTableNames;
-  for (const auto  : OptsWithMarshalling)
+  for (const auto  : MarshallingKindInfos)
 if (auto MaybeValueTableName = KindInfo->emitValueTable(OS))
   ValueTableNames.push_back(*MaybeValueTableName);
 
Index: llvm/include/llvm/Option/OptParser.td
===
--- llvm/include/llvm/Option/OptParser.td
+++ llvm/include/llvm/Option/OptParser.td
@@ -144,18 +144,24 @@
 
 // Helpers for defining marshalling information.
 
+class DefaultAnyOf defaults> {
+  code DefaultValue = !foldl("false", defaults, accumulator, option,
+ !strconcat(accumulator, " || ", !cast(option.KeyPath)));
+}
+
 class MarshallingInfo {
   code KeyPath = keypath;
   code DefaultValue = defaultvalue;
 }
+
 class MarshallingInfoString
   : MarshallingInfo {
   string MarshallingKind = "string";
   code NormalizerRetTy = normalizerretty;
 }
 
-class MarshallingInfoFlag
-  : MarshallingInfo {
+class MarshallingInfoFlag>
+  : MarshallingInfo {
   string MarshallingKind = "flag";
 }
 
Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -115,4 +115,20 @@
   ASSERT_THAT(GeneratedArgs, Each(StrNe(RelocationModelCStr)));
 }
 
+TEST_F(CC1CommandLineGenerationTest, CanGenerateCC1CommandLineImpliedFlags) {
+  const char *Args[] = {"clang", "-xc++", "-cl-unsafe-math-optimizations",
+"-cl-mad-enable", "-menable-unsafe-fp-math"};
+
+  CompilerInvocation CInvok;
+  CompilerInvocation::CreateFromArgs(CInvok, Args, *Diags);
+
+  CInvok.generateCC1CommandLine(GeneratedArgs, *this);
+
+  // Explicitly provided flags that were also implied by another flag, are not
+  // emitted.
+  ASSERT_THAT(GeneratedArgs, Contains(StrEq("-cl-unsafe-math-optimizations")));
+  ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-cl-mad-enable";
+  ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-menable-unsafe-fp-math";
+}
+
 } // anonymous namespace
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -941,15 +941,8 @@
   Opts.NoEscapingBlockTailCalls =
   Args.hasArg(OPT_fno_escaping_block_tail_calls);
   Opts.FloatABI = std::string(Args.getLastArgValue(OPT_mfloat_abi));
-  Opts.LessPreciseFPMAD = Args.hasArg(OPT_cl_mad_enable) ||
-  Args.hasArg(OPT_cl_unsafe_math_optimizations) ||
-  

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a subscriber: Anastasia.
jansvoboda11 added a comment.

Using the records with `DefaultAnyOf<[...]>` to catch errors seems to work 
well, thanks for the suggestion.

(I was initially thinking about using the keypaths to avoid errors and reorder 
the options in the TableGen backend to make everything work.)

@dexonsmith WDYT about the implementation?

@Anastasia There is no need to manually expand `-cc1` options anymore. Do you 
have any other concerns?


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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 300975.
jansvoboda11 added a comment.

Formalize the "implied by" relationship with `DefaultAnyOf<[...]>`.


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

https://reviews.llvm.org/D82756

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Option/OptParser.td
  llvm/utils/TableGen/OptParserEmitter.cpp

Index: llvm/utils/TableGen/OptParserEmitter.cpp
===
--- llvm/utils/TableGen/OptParserEmitter.cpp
+++ llvm/utils/TableGen/OptParserEmitter.cpp
@@ -435,7 +435,12 @@
   OS << "nullptr";
   };
 
-  std::vector> OptsWithMarshalling;
+  auto IsMarshallingOption = [](const Record ) {
+return !isa(R.getValueInit("MarshallingKind")) &&
+   !R.getValueAsString("KeyPath").empty();
+  };
+
+  std::vector OptsWithMarshalling;
   for (unsigned I = 0, E = Opts.size(); I != E; ++I) {
 const Record  = *Opts[I];
 
@@ -443,12 +448,31 @@
 OS << "OPTION(";
 WriteOptRecordFields(OS, R);
 OS << ")\n";
-if (!isa(R.getValueInit("MarshallingKind")))
-  OptsWithMarshalling.push_back(MarshallingKindInfo::create(R));
+if (IsMarshallingOption(R))
+  OptsWithMarshalling.push_back();
   }
   OS << "#endif // OPTION\n";
 
-  for (const auto  : OptsWithMarshalling) {
+  auto CmpMarshallingOpts = [](const Record *const *A, const Record *const *B) {
+unsigned AID = (*A)->getID();
+unsigned BID = (*B)->getID();
+
+if (AID < BID)
+  return -1;
+if (AID > BID)
+  return 1;
+return 0;
+  };
+
+  // Restore the definition order of marshalling options.
+  array_pod_sort(OptsWithMarshalling.begin(), OptsWithMarshalling.end(),
+ CmpMarshallingOpts);
+
+  std::vector> MarshallingKindInfos;
+  for (const auto *R : OptsWithMarshalling)
+MarshallingKindInfos.push_back(MarshallingKindInfo::create(*R));
+
+  for (const auto  : MarshallingKindInfos) {
 OS << "#ifdef " << KindInfo->MacroName << "\n";
 OS << KindInfo->MacroName << "(";
 WriteOptRecordFields(OS, KindInfo->R);
@@ -463,7 +487,7 @@
   OS << "\n";
   OS << MarshallingStringInfo::ValueTablePreamble;
   std::vector ValueTableNames;
-  for (const auto  : OptsWithMarshalling)
+  for (const auto  : MarshallingKindInfos)
 if (auto MaybeValueTableName = KindInfo->emitValueTable(OS))
   ValueTableNames.push_back(*MaybeValueTableName);
 
Index: llvm/include/llvm/Option/OptParser.td
===
--- llvm/include/llvm/Option/OptParser.td
+++ llvm/include/llvm/Option/OptParser.td
@@ -144,18 +144,24 @@
 
 // Helpers for defining marshalling information.
 
+class DefaultAnyOf defaults> {
+  code DefaultValue = !foldl("false", defaults, accumulator, option,
+ !strconcat(accumulator, " || ", !cast(option.KeyPath)));
+}
+
 class MarshallingInfo {
   code KeyPath = keypath;
   code DefaultValue = defaultvalue;
 }
+
 class MarshallingInfoString
   : MarshallingInfo {
   string MarshallingKind = "string";
   code NormalizerRetTy = normalizerretty;
 }
 
-class MarshallingInfoFlag
-  : MarshallingInfo {
+class MarshallingInfoFlag>
+  : MarshallingInfo {
   string MarshallingKind = "flag";
 }
 
Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -115,4 +115,19 @@
   ASSERT_THAT(GeneratedArgs, Each(StrNe(RelocationModelCStr)));
 }
 
+TEST_F(CC1CommandLineGenerationTest, CanGenerateCC1CommandLineImpliedFlags) {
+  const char *Args[] = {"clang", "-xc++", "-cl-unsafe-math-optimizations",
+"-cl-mad-enable", "-menable-unsafe-fp-math"};
+
+  CompilerInvocation CInvok;
+  CompilerInvocation::CreateFromArgs(CInvok, Args, *Diags);
+
+  CInvok.generateCC1CommandLine(GeneratedArgs, *this);
+
+  // Explicitly provided flags that can also be implied are not be emitted.
+  ASSERT_THAT(GeneratedArgs, Contains(StrEq("-cl-unsafe-math-optimizations")));
+  ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-cl-mad-enable";
+  ASSERT_THAT(GeneratedArgs, Not(Contains(StrEq("-menable-unsafe-fp-math";
+}
+
 } // anonymous namespace
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -941,15 +941,8 @@
   Opts.NoEscapingBlockTailCalls =
   Args.hasArg(OPT_fno_escaping_block_tail_calls);
   Opts.FloatABI = std::string(Args.getLastArgValue(OPT_mfloat_abi));
-  Opts.LessPreciseFPMAD = Args.hasArg(OPT_cl_mad_enable) ||
-  Args.hasArg(OPT_cl_unsafe_math_optimizations) ||
-  

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

IOW, the goal of formalizing would just be to:

- error if the `.td` file defined options in the wrong order to get correct 
parsing
- automatically generate the code for default value, instead of having to 
re-type the name of the keypath


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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D82756#2350233 , @jansvoboda11 
wrote:

> Correct me if I'm wrong, but when generating the command line, all "implied" 
> flags would be hidden, even if they were explicit in the original comand line:
>
> - original command line:  `clang -cc1 -cl-unsafe-math-optimizations 
> -cl-mad-enable -menable-unsafe-fp-math -mreassociate -fno-signed-zeros 
> -freciprocal-math -fapprox-func [...]`
> - generated command line: `clang -cc1 -cl-unsafe-math-optimizations [...]`
>
> This might be a bit surprising, but I don't think this would cause issues for 
> explicit modules. What are your thoughts?

I think this is fine. It's similar to a case where the caller might explicitly 
specify the default value, and it'll get canonicalized out.

> Formalizing the "implies" relationships would make it possible to remove the 
> ordering-sensitivity and possibly generate implied flags even when explicitly 
> passed to `cc1`. It would complicate the TableGen backend, which I'd prefer 
> to keep as simple as possible.

I was't thinking of dropping the ordering-sensitivity. Instead, you could just 
error if the referenced option hadn't declared already. One idea would be to 
change the tablegen to something like:

  MarshallingInfoFlag<
  "CodeGenOpts.LessPreciseFPMAD",
  DefaultAnyOf<[cl_unsafe_math_optimizations, cl_fast_relaxed_math]>>;

in the definition of `cl_mad_enable`, then:

- error if they aren't defined first; and
- construct a default value out of the key-paths.

I think this would less error-prone for maintenance, since it designs away some 
really subtle bugs.


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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

@dexonsmith, thanks for sharing your idea. Overall, I like the simplicity.

As you say, not every command line option has an associated `CodeGenOptions` or 
`LangOptions` field. For this patch only, we'd need to create the following 
fields:

- `CLUnsafeOptimizations` for `OPT_cl_unsafe_math_optimizations`
- `CLFiniteMathOnly` for `OPT_cl_finite_math_only`
- `CLNoSignedZeros` for `OPT_cl_no_signed_zeros`

I'm not sure how many other cases there are outside of what this patch touches.

Introducing ordering-sensitivity is not ideal, but if all options are defined 
in close proximity to each other, it should be relatively easy to reason about.

Correct me if I'm wrong, but when generating the command line, all "implied" 
flags would be hidden, even if they were explicit in the original comand line:

- original command line:  `clang -cc1 -cl-unsafe-math-optimizations 
-cl-mad-enable -menable-unsafe-fp-math -mreassociate -fno-signed-zeros 
-freciprocal-math -fapprox-func [...]`
- generated command line: `clang -cc1 -cl-unsafe-math-optimizations [...]`

This might be a bit surprising, but I don't think this would cause issues for 
explicit modules. What are your thoughts?

Formalizing the "implies" relationships would make it possible to remove the 
ordering-sensitivity and possibly generate implied flags even when explicitly 
passed to `cc1`. It would complicate the TableGen backend, which I'd prefer to 
keep as simple as possible.


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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D82756#2348159 , @dexonsmith wrote:

> I have an idea: use `DEFAULT_VALUE` to keep current behaviour. Here's an 
> example to demonstrate.

Downside of this is that it's error prone, because it requires for correctness 
(but does not enforce) a particular option definition order. The alternative 
would be to formalize it in the options somehow.


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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

I have an idea: use `DEFAULT_VALUE` to keep current behaviour. Here's an 
example to demonstrate.

`cl_mad_enable` is implied by `OPT_cl_unsafe_math_optimizations` or 
`OPT_cl_fast_relaxed_math`. Instead of setting the default value to `"false"` 
it could be set to `"CodeGenOpts.CLUnsafeMath || LangOpts.FastRelaxedMath"` 
(one hitch is that `CLUnsafeMath` doesn't currently exist).

You'd need to update the `OPTION_WITH_MARSHALLING_FLAG` definition in 
`parseSimpleArgs` to something like:

  #define OPTION_WITH_MARSHALLING_FLAG(PREFIX_TYPE, NAME, ID, KIND, GROUP,  
 \
   ALIAS, ALIASARGS, FLAGS, PARAM, 
HELPTEXT, \
   METAVAR, VALUES, SPELLING, ALWAYS_EMIT,  
 \
   KEYPATH, DEFAULT_VALUE, IS_POSITIVE) 
 \
this->KEYPATH = (Args.hasArg(OPT_##ID) && IS_POSITIVE) || (DEFAULT_VALUE);

@Bigcheese / @jansvoboda11 , WDYT?


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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia resigned from this revision.
Anastasia added a comment.

Just to clarify aside from the concern I have raised regarding internal testing 
I am not in any strong opposition of this feature. So if the community decides 
that it is more important to have this feature than to keep the tests short I 
am fine with this. However, due to the limited time I will not be able to 
continue reviewing this change as I don't want to block the progress due to my 
priorities.


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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/test/CodeGen/fp-function-attrs.cpp:2
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -ffast-math -ffinite-math-only 
-menable-unsafe-fp-math \
+// RUN:   -menable-no-infs -menable-no-nans -fno-signed-zeros 
-freciprocal-math \
+// RUN:   -fapprox-func -mreassociate -ffp-contract=fast -emit-llvm -o - %s | 
FileCheck %s

jansvoboda11 wrote:
> dang wrote:
> > Anastasia wrote:
> > > dang wrote:
> > > > Anastasia wrote:
> > > > > dang wrote:
> > > > > > Anastasia wrote:
> > > > > > > Not clear why do you need to pass these extra flags now?
> > > > > > Previously passing -ffast-math to CC1 implied all these other 
> > > > > > flags. I am trying to make CC1 option parsing as simple as 
> > > > > > possible, so that we can then make it easy to generate a command 
> > > > > > line from a CompilerInvocation instance. You can refer to [[ 
> > > > > > http://lists.llvm.org/pipermail/cfe-dev/2020-May/065421.html | 
> > > > > > http://lists.llvm.org/pipermail/cfe-dev/2020-May/065421.html ]] for 
> > > > > > more details on why we want to be able to do this
> > > > > Just to understand, there used to be implied flags and it made the 
> > > > > manual command line use of clang more compact and easy... Is the idea 
> > > > > now to change those compound flags such that individul flags always 
> > > > > need to be passed?
> > > > > 
> > > > > Although I thought you are still adding the implicit flags:
> > > > > 
> > > > >   {options::OPT_cl_fast_relaxed_math,
> > > > >[&](const Arg *Arg) {
> > > > >  RenderArg(Arg);
> > > > > 
> > > > >  
> > > > > CmdArgs.push_back(GetArgString(options::OPT_cl_mad_enable));
> > > > >  CmdArgs.push_back(GetArgString(options::OPT_ffast_math));
> > > > >  
> > > > > CmdArgs.push_back(GetArgString(options::OPT_ffinite_math_only));
> > > > >  CmdArgs.push_back(
> > > > >  GetArgString(options::OPT_menable_unsafe_fp_math));
> > > > >  
> > > > > CmdArgs.push_back(GetArgString(options::OPT_mreassociate));
> > > > >  
> > > > > CmdArgs.push_back(GetArgString(options::OPT_menable_no_nans));
> > > > >  CmdArgs.push_back(
> > > > >  GetArgString(options::OPT_menable_no_infinities));
> > > > >  
> > > > > CmdArgs.push_back(GetArgString(options::OPT_fno_signed_zeros));
> > > > >  
> > > > > CmdArgs.push_back(GetArgString(options::OPT_freciprocal_math));
> > > > >  
> > > > > CmdArgs.push_back(GetArgString(options::OPT_fapprox_func));
> > > > >}}
> > > > > 
> > > > > Do I just misunderstand something?
> > > > The command line of the driver doesn't change. This patch only affects 
> > > > what CC1 understands, now CC1 doesn't know anymore that 
> > > > `-cl-fast-relaxed-math` implies all these other options so the driver 
> > > > is responsible for specifying them when it constructs the CC1 command 
> > > > line.
> > > > 
> > > > To summarize, the clang driver command line isn't affected by this 
> > > > patch and it shouldn't be so let me know if something is wrong there. 
> > > > However, manually constructed `clang -cc1` invocations need to specify 
> > > > the all the implied flags manually now.
> > > Yes I understand, however, I am wondering whether this is intuitive 
> > > because it seems the behavior of clang with `-cc1` and without will be 
> > > different if the same `-cl-fast-relaxed-math` flag is passed.
> > > 
> > > I also find adding all the flags manually is too verbode if 
> > > `-cl-fast-relaxed-math` assumes to enable all the extra setting.
> > My understanding is that `-cc1` is an internal interface, so end-users 
> > should never use `-cc1` directly and/or rely on itss interface. It is 
> > already the case that flags mean very different things to the driver and 
> > `-cc1` for example "--target=" and "-triple". Furthermore, this impacted 
> > very few tests which leads me to believe that few compiler developers 
> > actually rely on this behavior.
> > 
> > Do you think this would be a major inconvenience to compiler developers to 
> > have to manually expand it out?
> Hi @Anastasia, I'll be taking over this patch. I agree with Daniel that 
> `-cc1` is an internal interface that doesn't need to match the public driver 
> interface.
> The current approach is by far the simplest to get command-line option 
> marshaling working.
> 
> What are your thoughts?
Sorry for the delay.

> My understanding is that -cc1 is an internal interface, so end-users should 
> never use -cc1 directly and/or rely on itss interface. 

This is true in practice but there are developers that use `-cc1` too. My main 
concern is however that the internal testing gets more complicated - with so 
many more flags to be added that can also be easy to miss.

Is there any compromise we could find?  


CHANGES SINCE LAST ACTION
  

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 26.
jansvoboda11 added a comment.

Rebase onto master.


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

https://reviews.llvm.org/D82756

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/complex-math.c
  clang/test/CodeGen/fast-math.c
  clang/test/CodeGen/finite-math.c
  clang/test/CodeGen/fp-floatcontrol-stack.cpp
  clang/test/CodeGen/fp-function-attrs.cpp
  clang/test/CodeGen/fp-options-to-fast-math-flags.c
  clang/test/CodeGen/fpconstrained.c
  clang/test/CodeGen/fpconstrained.cpp
  clang/test/CodeGen/libcalls.c
  clang/test/CodeGenOpenCL/no-signed-zeros.cl
  clang/test/CodeGenOpenCL/relaxed-fpmath.cl
  clang/test/Driver/opencl.cl
  clang/test/Headers/nvptx_device_math_sin.c
  clang/test/Headers/nvptx_device_math_sin.cpp
  clang/test/SemaOpenCL/fp-options.cl
  llvm/utils/TableGen/OptParserEmitter.cpp

Index: llvm/utils/TableGen/OptParserEmitter.cpp
===
--- llvm/utils/TableGen/OptParserEmitter.cpp
+++ llvm/utils/TableGen/OptParserEmitter.cpp
@@ -443,7 +443,8 @@
 OS << "OPTION(";
 WriteOptRecordFields(OS, R);
 OS << ")\n";
-if (!isa(R.getValueInit("MarshallingKind")))
+if (!isa(R.getValueInit("MarshallingKind")) &&
+!R.getValueAsString("KeyPath").empty())
   OptsWithMarshalling.push_back(MarshallingKindInfo::create(R));
   }
   OS << "#endif // OPTION\n";
Index: clang/test/SemaOpenCL/fp-options.cl
===
--- clang/test/SemaOpenCL/fp-options.cl
+++ clang/test/SemaOpenCL/fp-options.cl
@@ -1,4 +1,4 @@
 // RUN: %clang_cc1 %s -finclude-default-header -triple spir-unknown-unknown -emit-pch -o %t.pch
-// RUN: %clang_cc1 %s -finclude-default-header -cl-no-signed-zeros -triple spir-unknown-unknown -include-pch %t.pch -fsyntax-only -verify
+// RUN: %clang_cc1 %s -finclude-default-header -fno-signed-zeros -triple spir-unknown-unknown -include-pch %t.pch -fsyntax-only -verify
 // expected-no-diagnostics
 
Index: clang/test/Headers/nvptx_device_math_sin.cpp
===
--- clang/test/Headers/nvptx_device_math_sin.cpp
+++ clang/test/Headers/nvptx_device_math_sin.cpp
@@ -1,8 +1,15 @@
 // REQUIRES: nvptx-registered-target
 // RUN: %clang_cc1 -x c++ -internal-isystem %S/Inputs/include -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
 // RUN: %clang_cc1 -x c++ -include __clang_openmp_device_functions.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -internal-isystem %S/Inputs/include -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck %s --check-prefix=SLOW
-// RUN: %clang_cc1 -x c++ -internal-isystem %S/Inputs/include -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc -ffast-math -ffp-contract=fast
-// RUN: %clang_cc1 -x c++ -include __clang_openmp_device_functions.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -internal-isystem %S/Inputs/include -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - -ffast-math -ffp-contract=fast | FileCheck %s --check-prefix=FAST
+// RUN: %clang_cc1 -x c++ -internal-isystem %S/Inputs/include -fopenmp -triple powerpc64le-unknown-unknown \
+// RUN:   -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc -ffast-math -ffinite-math-only \
+// RUN:   -menable-no-infs -menable-no-nans -fno-signed-zeros -freciprocal-math -menable-unsafe-fp-math \
+// RUN:   -fapprox-func -mreassociate -ffp-contract=fast
+// RUN: %clang_cc1 -x c++ -include __clang_openmp_device_functions.h -internal-isystem %S/../../lib/Headers/openmp_wrappers \
+// RUN:   -internal-isystem %S/Inputs/include -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown \
+// RUN:   -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc \
+// RUN:   -o - -ffast-math -ffinite-math-only -menable-no-infs -menable-no-nans -fno-signed-zeros -freciprocal-math \
+// RUN:   -menable-unsafe-fp-math -fapprox-func -mreassociate -ffp-contract=fast | FileCheck %s --check-prefix=FAST
 // expected-no-diagnostics
 
 #include 
Index: clang/test/Headers/nvptx_device_math_sin.c
===
--- clang/test/Headers/nvptx_device_math_sin.c
+++ clang/test/Headers/nvptx_device_math_sin.c
@@ -1,8 +1,15 @@
 // REQUIRES: nvptx-registered-target
 // RUN: %clang_cc1 -x c -internal-isystem 

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.



Comment at: clang/test/CodeGen/fp-function-attrs.cpp:2
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -ffast-math -ffinite-math-only 
-menable-unsafe-fp-math \
+// RUN:   -menable-no-infs -menable-no-nans -fno-signed-zeros 
-freciprocal-math \
+// RUN:   -fapprox-func -mreassociate -ffp-contract=fast -emit-llvm -o - %s | 
FileCheck %s

dang wrote:
> Anastasia wrote:
> > dang wrote:
> > > Anastasia wrote:
> > > > dang wrote:
> > > > > Anastasia wrote:
> > > > > > Not clear why do you need to pass these extra flags now?
> > > > > Previously passing -ffast-math to CC1 implied all these other flags. 
> > > > > I am trying to make CC1 option parsing as simple as possible, so that 
> > > > > we can then make it easy to generate a command line from a 
> > > > > CompilerInvocation instance. You can refer to [[ 
> > > > > http://lists.llvm.org/pipermail/cfe-dev/2020-May/065421.html | 
> > > > > http://lists.llvm.org/pipermail/cfe-dev/2020-May/065421.html ]] for 
> > > > > more details on why we want to be able to do this
> > > > Just to understand, there used to be implied flags and it made the 
> > > > manual command line use of clang more compact and easy... Is the idea 
> > > > now to change those compound flags such that individul flags always 
> > > > need to be passed?
> > > > 
> > > > Although I thought you are still adding the implicit flags:
> > > > 
> > > >   {options::OPT_cl_fast_relaxed_math,
> > > >[&](const Arg *Arg) {
> > > >  RenderArg(Arg);
> > > > 
> > > >  
> > > > CmdArgs.push_back(GetArgString(options::OPT_cl_mad_enable));
> > > >  CmdArgs.push_back(GetArgString(options::OPT_ffast_math));
> > > >  
> > > > CmdArgs.push_back(GetArgString(options::OPT_ffinite_math_only));
> > > >  CmdArgs.push_back(
> > > >  GetArgString(options::OPT_menable_unsafe_fp_math));
> > > >  CmdArgs.push_back(GetArgString(options::OPT_mreassociate));
> > > >  
> > > > CmdArgs.push_back(GetArgString(options::OPT_menable_no_nans));
> > > >  CmdArgs.push_back(
> > > >  GetArgString(options::OPT_menable_no_infinities));
> > > >  
> > > > CmdArgs.push_back(GetArgString(options::OPT_fno_signed_zeros));
> > > >  
> > > > CmdArgs.push_back(GetArgString(options::OPT_freciprocal_math));
> > > >  CmdArgs.push_back(GetArgString(options::OPT_fapprox_func));
> > > >}}
> > > > 
> > > > Do I just misunderstand something?
> > > The command line of the driver doesn't change. This patch only affects 
> > > what CC1 understands, now CC1 doesn't know anymore that 
> > > `-cl-fast-relaxed-math` implies all these other options so the driver is 
> > > responsible for specifying them when it constructs the CC1 command line.
> > > 
> > > To summarize, the clang driver command line isn't affected by this patch 
> > > and it shouldn't be so let me know if something is wrong there. However, 
> > > manually constructed `clang -cc1` invocations need to specify the all the 
> > > implied flags manually now.
> > Yes I understand, however, I am wondering whether this is intuitive because 
> > it seems the behavior of clang with `-cc1` and without will be different if 
> > the same `-cl-fast-relaxed-math` flag is passed.
> > 
> > I also find adding all the flags manually is too verbode if 
> > `-cl-fast-relaxed-math` assumes to enable all the extra setting.
> My understanding is that `-cc1` is an internal interface, so end-users should 
> never use `-cc1` directly and/or rely on itss interface. It is already the 
> case that flags mean very different things to the driver and `-cc1` for 
> example "--target=" and "-triple". Furthermore, this impacted very few tests 
> which leads me to believe that few compiler developers actually rely on this 
> behavior.
> 
> Do you think this would be a major inconvenience to compiler developers to 
> have to manually expand it out?
Hi @Anastasia, I'll be taking over this patch. I agree with Daniel that `-cc1` 
is an internal interface that doesn't need to match the public driver interface.
The current approach is by far the simplest to get command-line option 
marshaling working.

What are your thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Daniel Grumberg via Phabricator via cfe-commits]

dang added a comment.

Ping?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Daniel Grumberg via Phabricator via cfe-commits]

dang added inline comments.



Comment at: clang/test/CodeGen/fp-function-attrs.cpp:2
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -ffast-math -ffinite-math-only 
-menable-unsafe-fp-math \
+// RUN:   -menable-no-infs -menable-no-nans -fno-signed-zeros 
-freciprocal-math \
+// RUN:   -fapprox-func -mreassociate -ffp-contract=fast -emit-llvm -o - %s | 
FileCheck %s

Anastasia wrote:
> dang wrote:
> > Anastasia wrote:
> > > dang wrote:
> > > > Anastasia wrote:
> > > > > Not clear why do you need to pass these extra flags now?
> > > > Previously passing -ffast-math to CC1 implied all these other flags. I 
> > > > am trying to make CC1 option parsing as simple as possible, so that we 
> > > > can then make it easy to generate a command line from a 
> > > > CompilerInvocation instance. You can refer to [[ 
> > > > http://lists.llvm.org/pipermail/cfe-dev/2020-May/065421.html | 
> > > > http://lists.llvm.org/pipermail/cfe-dev/2020-May/065421.html ]] for 
> > > > more details on why we want to be able to do this
> > > Just to understand, there used to be implied flags and it made the manual 
> > > command line use of clang more compact and easy... Is the idea now to 
> > > change those compound flags such that individul flags always need to be 
> > > passed?
> > > 
> > > Although I thought you are still adding the implicit flags:
> > > 
> > >   {options::OPT_cl_fast_relaxed_math,
> > >[&](const Arg *Arg) {
> > >  RenderArg(Arg);
> > > 
> > >  CmdArgs.push_back(GetArgString(options::OPT_cl_mad_enable));
> > >  CmdArgs.push_back(GetArgString(options::OPT_ffast_math));
> > >  
> > > CmdArgs.push_back(GetArgString(options::OPT_ffinite_math_only));
> > >  CmdArgs.push_back(
> > >  GetArgString(options::OPT_menable_unsafe_fp_math));
> > >  CmdArgs.push_back(GetArgString(options::OPT_mreassociate));
> > >  
> > > CmdArgs.push_back(GetArgString(options::OPT_menable_no_nans));
> > >  CmdArgs.push_back(
> > >  GetArgString(options::OPT_menable_no_infinities));
> > >  
> > > CmdArgs.push_back(GetArgString(options::OPT_fno_signed_zeros));
> > >  
> > > CmdArgs.push_back(GetArgString(options::OPT_freciprocal_math));
> > >  CmdArgs.push_back(GetArgString(options::OPT_fapprox_func));
> > >}}
> > > 
> > > Do I just misunderstand something?
> > The command line of the driver doesn't change. This patch only affects what 
> > CC1 understands, now CC1 doesn't know anymore that `-cl-fast-relaxed-math` 
> > implies all these other options so the driver is responsible for specifying 
> > them when it constructs the CC1 command line.
> > 
> > To summarize, the clang driver command line isn't affected by this patch 
> > and it shouldn't be so let me know if something is wrong there. However, 
> > manually constructed `clang -cc1` invocations need to specify the all the 
> > implied flags manually now.
> Yes I understand, however, I am wondering whether this is intuitive because 
> it seems the behavior of clang with `-cc1` and without will be different if 
> the same `-cl-fast-relaxed-math` flag is passed.
> 
> I also find adding all the flags manually is too verbode if 
> `-cl-fast-relaxed-math` assumes to enable all the extra setting.
My understanding is that `-cc1` is an internal interface, so end-users should 
never use `-cc1` directly and/or rely on itss interface. It is already the case 
that flags mean very different things to the driver and `-cc1` for example 
"--target=" and "-triple". Furthermore, this impacted very few tests which 
leads me to believe that few compiler developers actually rely on this behavior.

Do you think this would be a major inconvenience to compiler developers to have 
to manually expand it out?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/test/CodeGen/fp-function-attrs.cpp:2
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -ffast-math -ffinite-math-only 
-menable-unsafe-fp-math \
+// RUN:   -menable-no-infs -menable-no-nans -fno-signed-zeros 
-freciprocal-math \
+// RUN:   -fapprox-func -mreassociate -ffp-contract=fast -emit-llvm -o - %s | 
FileCheck %s

dang wrote:
> Anastasia wrote:
> > dang wrote:
> > > Anastasia wrote:
> > > > Not clear why do you need to pass these extra flags now?
> > > Previously passing -ffast-math to CC1 implied all these other flags. I am 
> > > trying to make CC1 option parsing as simple as possible, so that we can 
> > > then make it easy to generate a command line from a CompilerInvocation 
> > > instance. You can refer to [[ 
> > > http://lists.llvm.org/pipermail/cfe-dev/2020-May/065421.html | 
> > > http://lists.llvm.org/pipermail/cfe-dev/2020-May/065421.html ]] for more 
> > > details on why we want to be able to do this
> > Just to understand, there used to be implied flags and it made the manual 
> > command line use of clang more compact and easy... Is the idea now to 
> > change those compound flags such that individul flags always need to be 
> > passed?
> > 
> > Although I thought you are still adding the implicit flags:
> > 
> >   {options::OPT_cl_fast_relaxed_math,
> >[&](const Arg *Arg) {
> >  RenderArg(Arg);
> > 
> >  CmdArgs.push_back(GetArgString(options::OPT_cl_mad_enable));
> >  CmdArgs.push_back(GetArgString(options::OPT_ffast_math));
> >  
> > CmdArgs.push_back(GetArgString(options::OPT_ffinite_math_only));
> >  CmdArgs.push_back(
> >  GetArgString(options::OPT_menable_unsafe_fp_math));
> >  CmdArgs.push_back(GetArgString(options::OPT_mreassociate));
> >  CmdArgs.push_back(GetArgString(options::OPT_menable_no_nans));
> >  CmdArgs.push_back(
> >  GetArgString(options::OPT_menable_no_infinities));
> >  CmdArgs.push_back(GetArgString(options::OPT_fno_signed_zeros));
> >  CmdArgs.push_back(GetArgString(options::OPT_freciprocal_math));
> >  CmdArgs.push_back(GetArgString(options::OPT_fapprox_func));
> >}}
> > 
> > Do I just misunderstand something?
> The command line of the driver doesn't change. This patch only affects what 
> CC1 understands, now CC1 doesn't know anymore that `-cl-fast-relaxed-math` 
> implies all these other options so the driver is responsible for specifying 
> them when it constructs the CC1 command line.
> 
> To summarize, the clang driver command line isn't affected by this patch and 
> it shouldn't be so let me know if something is wrong there. However, manually 
> constructed `clang -cc1` invocations need to specify the all the implied 
> flags manually now.
Yes I understand, however, I am wondering whether this is intuitive because it 
seems the behavior of clang with `-cc1` and without will be different if the 
same `-cl-fast-relaxed-math` flag is passed.

I also find adding all the flags manually is too verbode if 
`-cl-fast-relaxed-math` assumes to enable all the extra setting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82756

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Daniel Grumberg via Phabricator via cfe-commits]

dang marked an inline comment as done.
dang added inline comments.



Comment at: clang/test/CodeGen/fp-function-attrs.cpp:2
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -ffast-math -ffinite-math-only 
-menable-unsafe-fp-math \
+// RUN:   -menable-no-infs -menable-no-nans -fno-signed-zeros 
-freciprocal-math \
+// RUN:   -fapprox-func -mreassociate -ffp-contract=fast -emit-llvm -o - %s | 
FileCheck %s

Anastasia wrote:
> dang wrote:
> > Anastasia wrote:
> > > Not clear why do you need to pass these extra flags now?
> > Previously passing -ffast-math to CC1 implied all these other flags. I am 
> > trying to make CC1 option parsing as simple as possible, so that we can 
> > then make it easy to generate a command line from a CompilerInvocation 
> > instance. You can refer to [[ 
> > http://lists.llvm.org/pipermail/cfe-dev/2020-May/065421.html | 
> > http://lists.llvm.org/pipermail/cfe-dev/2020-May/065421.html ]] for more 
> > details on why we want to be able to do this
> Just to understand, there used to be implied flags and it made the manual 
> command line use of clang more compact and easy... Is the idea now to change 
> those compound flags such that individul flags always need to be passed?
> 
> Although I thought you are still adding the implicit flags:
> 
>   {options::OPT_cl_fast_relaxed_math,
>[&](const Arg *Arg) {
>  RenderArg(Arg);
> 
>  CmdArgs.push_back(GetArgString(options::OPT_cl_mad_enable));
>  CmdArgs.push_back(GetArgString(options::OPT_ffast_math));
>  CmdArgs.push_back(GetArgString(options::OPT_ffinite_math_only));
>  CmdArgs.push_back(
>  GetArgString(options::OPT_menable_unsafe_fp_math));
>  CmdArgs.push_back(GetArgString(options::OPT_mreassociate));
>  CmdArgs.push_back(GetArgString(options::OPT_menable_no_nans));
>  CmdArgs.push_back(
>  GetArgString(options::OPT_menable_no_infinities));
>  CmdArgs.push_back(GetArgString(options::OPT_fno_signed_zeros));
>  CmdArgs.push_back(GetArgString(options::OPT_freciprocal_math));
>  CmdArgs.push_back(GetArgString(options::OPT_fapprox_func));
>}}
> 
> Do I just misunderstand something?
The command line of the driver doesn't change. This patch only affects what CC1 
understands, now CC1 doesn't know anymore that `-cl-fast-relaxed-math` implies 
all these other options so the driver is responsible for specifying them when 
it constructs the CC1 command line.

To summarize, the clang driver command line isn't affected by this patch and it 
shouldn't be so let me know if something is wrong there. However, manually 
constructed `clang -cc1` invocations need to specify the all the implied flags 
manually now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82756



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/test/CodeGen/fp-function-attrs.cpp:2
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -ffast-math -ffinite-math-only 
-menable-unsafe-fp-math \
+// RUN:   -menable-no-infs -menable-no-nans -fno-signed-zeros 
-freciprocal-math \
+// RUN:   -fapprox-func -mreassociate -ffp-contract=fast -emit-llvm -o - %s | 
FileCheck %s

dang wrote:
> Anastasia wrote:
> > Not clear why do you need to pass these extra flags now?
> Previously passing -ffast-math to CC1 implied all these other flags. I am 
> trying to make CC1 option parsing as simple as possible, so that we can then 
> make it easy to generate a command line from a CompilerInvocation instance. 
> You can refer to [[ 
> http://lists.llvm.org/pipermail/cfe-dev/2020-May/065421.html | 
> http://lists.llvm.org/pipermail/cfe-dev/2020-May/065421.html ]] for more 
> details on why we want to be able to do this
Just to understand, there used to be implied flags and it made the manual 
command line use of clang more compact and easy... Is the idea now to change 
those compound flags such that individul flags always need to be passed?

Although I thought you are still adding the implicit flags:

  {options::OPT_cl_fast_relaxed_math,
   [&](const Arg *Arg) {
 RenderArg(Arg);

 CmdArgs.push_back(GetArgString(options::OPT_cl_mad_enable));
 CmdArgs.push_back(GetArgString(options::OPT_ffast_math));
 CmdArgs.push_back(GetArgString(options::OPT_ffinite_math_only));
 CmdArgs.push_back(
 GetArgString(options::OPT_menable_unsafe_fp_math));
 CmdArgs.push_back(GetArgString(options::OPT_mreassociate));
 CmdArgs.push_back(GetArgString(options::OPT_menable_no_nans));
 CmdArgs.push_back(
 GetArgString(options::OPT_menable_no_infinities));
 CmdArgs.push_back(GetArgString(options::OPT_fno_signed_zeros));
 CmdArgs.push_back(GetArgString(options::OPT_freciprocal_math));
 CmdArgs.push_back(GetArgString(options::OPT_fapprox_func));
   }}

Do I just misunderstand something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82756



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Daniel Grumberg via Phabricator via cfe-commits]

dang added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1176
+defm reciprocal_math : OptInFFlag< "reciprocal-math", "Allow division 
operations to be reassociated", "", "", [], "LangOpts->AllowRecip">;
+def fapprox_func : Flag<["-"], "fapprox-func">, Group, 
Flags<[CC1Option, NoDriverOption]>,
+  MarshallingInfoFlag<"LangOpts->ApproxFunc", "false">;

Anastasia wrote:
> could this also be OptInFFlag?
The aim was to keep the driver semantics the same as before and this was not 
something you could control with the driver, so I left it as just a CC1 flag. 
However if it makes sense to be able to control this from the driver then we 
can definitely make this `OptInFFLag`.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2805
 CmdArgs.push_back("-menable-unsafe-fp-math");
+ApproxFunc = true;
+  }

Anastasia wrote:
> Is this a bug fix ?
No, in current trunk approximating floating point functions was something that 
was implied by other optimization flags, i.e. disabling math errno, enabling 
associative/reciprocal math, disabling signed zeros and disabling trapping math 
and -ffast-math which does all the previously mentioned things. This patch 
moves this logic in the driver by introducing a new CC1 flag for this so that 
parsing CC1 options can be more easily automated. This just reflects the logic 
that was previously inside cc1.



Comment at: clang/test/CodeGen/fp-function-attrs.cpp:2
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -ffast-math -ffinite-math-only 
-menable-unsafe-fp-math \
+// RUN:   -menable-no-infs -menable-no-nans -fno-signed-zeros 
-freciprocal-math \
+// RUN:   -fapprox-func -mreassociate -ffp-contract=fast -emit-llvm -o - %s | 
FileCheck %s

Anastasia wrote:
> Not clear why do you need to pass these extra flags now?
Previously passing -ffast-math to CC1 implied all these other flags. I am 
trying to make CC1 option parsing as simple as possible, so that we can then 
make it easy to generate a command line from a CompilerInvocation instance. You 
can refer to [[ http://lists.llvm.org/pipermail/cfe-dev/2020-May/065421.html | 
http://lists.llvm.org/pipermail/cfe-dev/2020-May/065421.html ]] for more 
details on why we want to be able to do this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82756



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: clang/include/clang/Driver/Options.td:1176
+defm reciprocal_math : OptInFFlag< "reciprocal-math", "Allow division 
operations to be reassociated", "", "", [], "LangOpts->AllowRecip">;
+def fapprox_func : Flag<["-"], "fapprox-func">, Group, 
Flags<[CC1Option, NoDriverOption]>,
+  MarshallingInfoFlag<"LangOpts->ApproxFunc", "false">;

could this also be OptInFFlag?



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2805
 CmdArgs.push_back("-menable-unsafe-fp-math");
+ApproxFunc = true;
+  }

Is this a bug fix ?



Comment at: clang/test/CodeGen/fp-function-attrs.cpp:2
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -ffast-math -ffinite-math-only 
-menable-unsafe-fp-math \
+// RUN:   -menable-no-infs -menable-no-nans -fno-signed-zeros 
-freciprocal-math \
+// RUN:   -fapprox-func -mreassociate -ffp-contract=fast -emit-llvm -o - %s | 
FileCheck %s

Not clear why do you need to pass these extra flags now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82756



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82756: Port some floating point options to new option marshalling infrastructure

[Daniel Grumberg via Phabricator via cfe-commits]

dang created this revision.
dang added a reviewer: Bigcheese.
Herald added subscribers: llvm-commits, cfe-commits, sstefan1, dexonsmith.
Herald added a reviewer: jdoerfert.
Herald added projects: clang, LLVM.
dang added a parent revision: D82574: Merge TableGen files used for clang 
options.

This changes cc1 semantics for some options such as `-cl-fast-relaxed-math` 
that implied other options. Now the driver always emits all the implied 
options, and each option maps to one key path in `CompilerInvocation` so that 
the new option parsing system can be used.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82756

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/complex-math.c
  clang/test/CodeGen/fast-math.c
  clang/test/CodeGen/finite-math.c
  clang/test/CodeGen/fp-floatcontrol-stack.cpp
  clang/test/CodeGen/fp-function-attrs.cpp
  clang/test/CodeGen/fp-options-to-fast-math-flags.c
  clang/test/CodeGen/fpconstrained.c
  clang/test/CodeGen/fpconstrained.cpp
  clang/test/CodeGen/libcalls.c
  clang/test/CodeGenOpenCL/no-signed-zeros.cl
  clang/test/CodeGenOpenCL/relaxed-fpmath.cl
  clang/test/Driver/opencl.cl
  clang/test/Headers/nvptx_device_math_sin.c
  clang/test/Headers/nvptx_device_math_sin.cpp
  llvm/utils/TableGen/OptParserEmitter.cpp

Index: llvm/utils/TableGen/OptParserEmitter.cpp
===
--- llvm/utils/TableGen/OptParserEmitter.cpp
+++ llvm/utils/TableGen/OptParserEmitter.cpp
@@ -435,7 +435,8 @@
 OS << "OPTION(";
 WriteOptRecordFields(OS, R);
 OS << ")\n";
-if (!isa(R.getValueInit("MarshallingKind")))
+if (!isa(R.getValueInit("MarshallingKind")) &&
+!R.getValueAsString("KeyPath").empty())
   OptsWithMarshalling.push_back(MarshallingKindInfo::create(R));
   }
   OS << "#endif // OPTION\n";
Index: clang/test/Headers/nvptx_device_math_sin.cpp
===
--- clang/test/Headers/nvptx_device_math_sin.cpp
+++ clang/test/Headers/nvptx_device_math_sin.cpp
@@ -1,8 +1,15 @@
 // REQUIRES: nvptx-registered-target
 // RUN: %clang_cc1 -x c++ -internal-isystem %S/Inputs/include -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
 // RUN: %clang_cc1 -x c++ -include __clang_openmp_device_functions.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -internal-isystem %S/Inputs/include -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - | FileCheck %s --check-prefix=SLOW
-// RUN: %clang_cc1 -x c++ -internal-isystem %S/Inputs/include -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc -ffast-math -ffp-contract=fast
-// RUN: %clang_cc1 -x c++ -include __clang_openmp_device_functions.h -internal-isystem %S/../../lib/Headers/openmp_wrappers -internal-isystem %S/Inputs/include -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc -o - -ffast-math -ffp-contract=fast | FileCheck %s --check-prefix=FAST
+// RUN: %clang_cc1 -x c++ -internal-isystem %S/Inputs/include -fopenmp -triple powerpc64le-unknown-unknown \
+// RUN:   -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc -ffast-math -ffinite-math-only \
+// RUN:   -menable-no-infs -menable-no-nans -fno-signed-zeros -freciprocal-math -menable-unsafe-fp-math \
+// RUN:   -fapprox-func -mreassociate -ffp-contract=fast
+// RUN: %clang_cc1 -x c++ -include __clang_openmp_device_functions.h -internal-isystem %S/../../lib/Headers/openmp_wrappers \
+// RUN:   -internal-isystem %S/Inputs/include -fopenmp -triple nvptx64-nvidia-cuda -aux-triple powerpc64le-unknown-unknown \
+// RUN:   -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm %s -fopenmp-is-device -fopenmp-host-ir-file-path %t-ppc-host.bc \
+// RUN:   -o - -ffast-math -ffinite-math-only -menable-no-infs -menable-no-nans -fno-signed-zeros -freciprocal-math \
+// RUN:   -menable-unsafe-fp-math -fapprox-func -mreassociate -ffp-contract=fast | FileCheck %s --check-prefix=FAST
 // expected-no-diagnostics
 
 #include 
Index: clang/test/Headers/nvptx_device_math_sin.c
===
--- clang/test/Headers/nvptx_device_math_sin.c
+++ clang/test/Headers/nvptx_device_math_sin.c
@@ -1,8 +1,15 @@
 // REQUIRES: nvptx-registered-target
 // RUN: %clang_cc1 -x c -internal-isystem %S/Inputs/include -fopenmp -triple powerpc64le-unknown-unknown -fopenmp-targets=nvptx64-nvidia-cuda -emit-llvm-bc %s -o %t-ppc-host.bc
 // RUN: %clang_cc1 -x c -include __clang_openmp_device_functions.h -internal-isystem