Page MenuHomePhabricator

Port some floating point options to new option marshalling infrastructure
ClosedPublic

Authored by jansvoboda11 on Jun 29 2020, 4:02 AM.

Details

Summary

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.

Diff Detail

Event Timeline

dang created this revision.Jun 29 2020, 4:02 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
jdoerfert resigned from this revision.Jun 29 2020, 5:11 PM
Anastasia added inline comments.Jul 8 2020, 6:31 AM
clang/include/clang/Driver/Options.td
1236

could this also be OptInFFlag?

clang/lib/Driver/ToolChains/Clang.cpp
2805 ↗(On Diff #274048)

Is this a bug fix ?

clang/test/CodeGen/fp-function-attrs.cpp
2 ↗(On Diff #274048)

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

dang added inline comments.Jul 9 2020, 2:21 AM
clang/include/clang/Driver/Options.td
1236

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.

clang/lib/Driver/ToolChains/Clang.cpp
2805 ↗(On Diff #274048)

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.

clang/test/CodeGen/fp-function-attrs.cpp
2 ↗(On Diff #274048)

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 for more details on why we want to be able to do this

Anastasia added inline comments.Jul 17 2020, 9:46 AM
clang/test/CodeGen/fp-function-attrs.cpp
2 ↗(On Diff #274048)

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?

dang marked an inline comment as done.Jul 27 2020, 10:28 AM
dang added inline comments.
clang/test/CodeGen/fp-function-attrs.cpp
2 ↗(On Diff #274048)

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.

Anastasia added inline comments.Aug 5 2020, 4:09 AM
clang/test/CodeGen/fp-function-attrs.cpp
2 ↗(On Diff #274048)

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.

dang added inline comments.Aug 5 2020, 10:40 AM
clang/test/CodeGen/fp-function-attrs.cpp
2 ↗(On Diff #274048)

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?

jansvoboda11 added inline comments.
clang/test/CodeGen/fp-function-attrs.cpp
2 ↗(On Diff #274048)

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?

jansvoboda11 commandeered this revision.Oct 22 2020, 8:44 AM
jansvoboda11 added a reviewer: dang.

Rebase onto master.

Anastasia added inline comments.Oct 22 2020, 10:42 AM
clang/test/CodeGen/fp-function-attrs.cpp
2 ↗(On Diff #274048)

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?

Anastasia resigned from this revision.Oct 22 2020, 11:02 AM

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.

dexonsmith added a comment.EditedOct 22 2020, 11:59 AM

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?

Edit: Note that generating the -cc1 should already mostly work, you'd just want to add () around DEFAULT_VALUE:

#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)      \
  if ((FLAGS) & options::CC1Option &&                                            \
      (ALWAYS_EMIT || this->KEYPATH != (DEFAULT_VALUE)))                         \
    Args.push_back(SPELLING);

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.

@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.

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.

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

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

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?

Fix typo & whitespace.

dexonsmith requested changes to this revision.Oct 27 2020, 9:03 AM

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/.

clang/include/clang/Driver/Options.td
1236

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.

clang/lib/Frontend/CompilerInvocation.cpp
3745

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).

llvm/utils/TableGen/OptParserEmitter.cpp
460–464

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();
468–469

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, ...));
This revision now requires changes to proceed.Oct 27 2020, 9:03 AM

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.

clang/include/clang/Driver/Options.td
1236

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?

clang/lib/Frontend/CompilerInvocation.cpp
3745

Got it.

llvm/utils/TableGen/OptParserEmitter.cpp
460–464

I see that array_pod_sort calls qsort from the C standard library, which should use the result of comparator as an int.

468–469
  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.
  1. Later, they are reordered in this function before prefix groups are generated: array_pod_sort(Opts.begin(), Opts.end(), CompareOptionRecords);.
  1. 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.

Integrated code review suggestions.

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.

clang/include/clang/Driver/Options.td
1236

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.

llvm/utils/TableGen/OptParserEmitter.cpp
460–464

Thanks, you're right, I misremembered array_pod_sort somehow reinterpreting the lambda...

468–469

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...)

Added LLVM unit tests, reverted back to array_pod_sort.

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

clang/include/clang/Driver/Options.td
1236

Agreed.

llvm/utils/TableGen/OptParserEmitter.cpp
468–469

No problem. I've switched back to array_pod_sort.

dexonsmith accepted this revision.Oct 30 2020, 9:04 AM

LGTM, thanks for working through this.

This revision is now accepted and ready to land.Oct 30 2020, 9:04 AM

Thanks for the feedback.

@Bigcheese do you have anything to add?

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

@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?

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

dexonsmith added a comment.EditedMon, Nov 9, 1:16 PM

Thanks, Jan Svoboda and jan_svoboda@apple.com is fine. Is it possible to add @dang as a co-author? git log says he uses Daniel Grumberg and dany.grumberg@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, Opt{In,Out}FFlag were enhanced to allow other flags to
imply them, 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.