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

Thu Oct 29 08:55:43 PDT 2020

dexonsmith added a comment.

In D82756#2361565 <https://reviews.llvm.org/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<f_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 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::string, std::unique_ptr<Record>, 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 <https://llvm.org/doxygen/namespacellvm.html#add1eb5637dd671428b6f138ed3db6428>.
Thanks for the explanation about the ordering, this makes sense.

Regarding `array_pod_sort`, I was referring to how `llvm::sort` calls `array_pod_sort` when it can... but I hadn't noticed before that it can't do this for a custom comparator. You should stick with `array_pod_sort` (although maybe as a follow-up I'll look into whether we can detect if the custom comparator to `llvm::sort` is stateless and defer to `array_pod_sort` in that case as well...)

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

https://reviews.llvm.org/D82756