[Playgrounds] Add a new `-playground-option` flag to control which transforms to apply #69355

abertelrud · 2023-10-24T02:57:00Z

Generalize the existing -playground-high-performance flag into a set of options that control various aspects of the "playground transformation" in Sema. The intent is for this to be a scalable way to control a more fine-grained set of upcoming options.

Discussion

This commit adds the first two of those controllable parts of the transform, matching what the existing flag already controls (scope entry/exit and function arguments), but in an extensible way.

So instead of a single flag, we represent the playground transform options as a set of well-defined choices, with a new -playground-option flag to individually enable or disable those options (when prefixed with "No", the corresponding option is instead disabled). Enabling an already-enabled option or disabling an already-disabled option is a no-op.

For compatibility, the existing -playground-high-performance flag causes "expensive" transforms to be disabled, as before. We can also leave it as a useful shorthand to include or exclude new options even in the future, based on their cost.

The machinery for implementing this is similar to how Features.def works, with a new PlaygroundOptions.def that defines the supported playground transform options. Each playground definition specifies the name and description, as well as whether the option is enabled by default, and whether it's also enabled in the "high performance" case.

Adding a new option in the future only requires adding it to PlaygroundOptions.def, deciding whether it should be on or off by default, deciding whether it should also be on or off in -playground-high-performance mode, and checking for its presence from the appropriate places in PlaygroundTransform.cpp.

Note that this is intended to control the types of user-visible results that the invoker of the compiler wants, from an externally detectable standpoint. Other flags, such as whether or not to use the extended form of the callbacks, remain as experimental features, since those deal with the mechanics and not the desired observed behavior.

Changes

add a PlaygroundOption.h that allows individual playground transform options to be defined
add a PlaygroundOptions.def that defines the available playground transform options
replace the "high performance mode" flag in LangOptions with a PlaygroundOptionSet
add a -playground-option flag to FrontendOptions.td
modify CompilerInvocation.cpp to apply the playground options to the set
- the default value of each option, and whether "high performance mode affects it", is determined by its definition
modify PlaygroundTransform.cpp to check for the new options in the set
add a new unit test that checks that -playground-option transforms can be enabled
extend the existing unit test for "high performance" mode to check that this can also be achieved by disabled corresponding options

rdar://109911673

…ansforms to apply Generalize the existing `-playground-high-performance` flag into a set of options that control various aspects of the "playground transformation" in Sema. This commit adds the first two of those controllable parts of the transform, matching what the existing flag already controls (scope entry/exit and function arguments), but in an extensible way. The intent is for this to be a scalable way to control a larger set of upcoming options. So instead of a single flag, we represent the playground transform options as a set of well-defined choices, with a new `-playground-option` flag to individually enable or disable those options (when prefixed with "No", the corresponding option is instead disabled). Enabling an already-enabled option or disabling an already-disabled option is a no-op. For compatibility, the existing `-playground-high-performance` flag causes "expensive" transforms to be disabled, as before. We can also leave it as a useful shorthand to include or exclude new options even in the future, based on their cost. The machinery for implementing this is similar to how `Features.def` works, with a new `PlaygroundOptions.def` that defines the supported playground transform options. Each playground definition specifies the name and description, as well as whether the option is enabled by default, and whether it's also enabled in the "high performance" case. Adding a new option in the future only requires adding it to `PlaygroundOptions.def`, deciding whether it should be on or off by default, deciding whether it should also be on or off in `-playground-high-performance` mode, and checking for its presence from the appropriate places in `PlaygroundTransform.cpp`. Note that this is intended to control the types of user-visible results that the invoker of the compiler wants, from an externally detectable standpoint. Other flags, such as whether or not to use the extended form of the callbacks, remain as experimental features, since those deal with the mechanics and not the desired observed behavior. rdar://109911673

abertelrud · 2023-10-24T02:57:21Z

@swift-ci please test

abertelrud · 2023-10-24T02:59:06Z

@swift-ci please smoke test

abertelrud · 2023-10-24T17:18:57Z

Test failure seems to have been due to some authorization token (i.e. infrastructure failure). Trying again.

abertelrud · 2023-10-24T17:19:04Z

@swift-ci please smoke test

abertelrud · 2023-10-24T17:57:50Z

Test failure seems to have been due to some authorization token (i.e. infrastructure failure). Trying again.

That was just the full test. The smoke test seems to be because LLDB calls into one of the methods I've changed, so I'll need to put in a compatibility function to avoid revlock.

abertelrud · 2023-11-06T19:12:33Z

@swift-ci please smoke test

abertelrud · 2023-11-08T19:30:22Z

Added back the older variant of the member function for compatibility with LLDB and any other callers.

artemcm

This looks great!

artemcm · 2023-11-13T19:30:44Z

include/swift/Subsystems.h

+  /// Once type checking is complete, this optionally transforms the ASTs to
+  /// insert calls to external logging functions. This function is provided
+  /// for backward compatibility with existing code; for new code, the variant
+  /// that takes an `PlaygroundOptionSet` parameter should be used.
  ///
  /// \param HighPerformance True if the playground transform should omit
  /// instrumentation that has a high runtime performance impact.
  void performPlaygroundTransform(SourceFile &SF, bool HighPerformance);


Worth a comment to call out this method as deprecated and to be removed in the near future.

Good point — I'll add that as a comment.

Updated the PR with a comment.

artemcm · 2023-11-13T19:31:34Z

test/PlaygroundTransform/playground_options.swift

+// RUN: %target-codesign %t/main
+// RUN: %target-run %t/main | %FileCheck %s
+//
+// RUN: %target-build-swift -Xfrontend -playground -Xfrontend -playground-high-performance -Xfrontend -playground-option -Xfrontend FunctionParameters -Xfrontend -playground-option -Xfrontend ScopeEvents -Xfrontend -playground-option -Xfrontend ThisOptionShouldBeGracefullyIgnored -o %t/main2 -I=%t %t/PlaygroundSupport.o %t/main.swift


Will we want to upgrade -playground-option to an official driver flag?

I'm not sure — I would think so. Would that be a separate PR on SwiftDriver?

Having this be a driver option does sound more ergonomic. One of the future changes I have lined up to propose as a PR is to have a way to print the available playground transform options, and that definitely feels as if it belongs in the driver (with the data coming from the frontend of course).

We would need to move the option to Options.td and then yeah have a separate PR against swift-driver following these instructions:
https://github.com/apple/swift-driver#rebuilding-optionsswift

Thank you! I'll take a look at that.

I've taken a closer look here, it looks to me as if all the other playground options are currently just frontend flags (including the -playground option itself that enables the transform). So it looks to me as if enabling the playground-related flags as driver flags should be a separate PR, which would enable -playground as well as -playground-option (and ideally also a future new -playground-print-options that would list the supported options, though that would be yet another separate PR). @artemcm WDYT?

… takes a high-perf flag Temporarily restore the old variant of the `performPlaygroundTransform()` that takes a boolean to determine whether to only apply high-performance playground transforms. This function is still used by LLDB and possibly other clients, and should continue to work. It can be removed once all clients have switched over to the more generic variant that takes a `PlaygroundOptionSet`. Also added a comment to the old function indicating that new code should use the more general form.

abertelrud · 2023-11-15T17:16:03Z

@swift-ci please smoke test

abertelrud requested review from hborla, slavapestov, xedin and tshortli as code owners October 24, 2023 02:57

abertelrud self-assigned this Oct 24, 2023

abertelrud requested a review from artemcm October 24, 2023 17:18

abertelrud marked this pull request as draft October 24, 2023 17:57

abertelrud marked this pull request as ready for review November 8, 2023 19:30

artemcm approved these changes Nov 13, 2023

View reviewed changes

abertelrud force-pushed the playground-transform-option-flag branch from 8ce38c6 to 65efc1f Compare November 15, 2023 17:15

abertelrud merged commit a363603 into swiftlang:main Nov 15, 2023

abertelrud deleted the playground-transform-option-flag branch November 15, 2023 21:02

[Playgrounds] Add a new -playground-option flag to control which transforms to apply #69355

[Playgrounds] Add a new -playground-option flag to control which transforms to apply #69355

Uh oh!

Conversation

abertelrud commented Oct 24, 2023

Discussion

Changes

Uh oh!

abertelrud commented Oct 24, 2023

Uh oh!

abertelrud commented Oct 24, 2023

Uh oh!

abertelrud commented Oct 24, 2023

Uh oh!

abertelrud commented Oct 24, 2023

Uh oh!

abertelrud commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abertelrud commented Nov 6, 2023

Uh oh!

abertelrud commented Nov 8, 2023

Uh oh!

artemcm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abertelrud Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abertelrud commented Nov 15, 2023

Uh oh!

Uh oh!

[Playgrounds] Add a new `-playground-option` flag to control which transforms to apply #69355

[Playgrounds] Add a new `-playground-option` flag to control which transforms to apply #69355

abertelrud commented Oct 24, 2023 •

edited

Loading

abertelrud Nov 15, 2023 •

edited

Loading