-
-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add --profiling flag #1065
Add --profiling flag #1065
Conversation
- Followup to #1025 - Required for #1059 This flag is proposed in the ADR for the OutputManager but wasn't implemented in #1025, which left us without a way to enable the generation of the profile-rules.txt file that used to always be created (other than using the global config). The benchmarks depend on this file being created.
// Flags can be passed from options too, e.g. --profile or --write-intermediate | ||
// Strangly, we currently require data to flow both from the pass options | ||
// to the output manager and from the manager to the pass options, this this "sync" | ||
// is by directional. | ||
// TODO: remove this once all flag operations are moved into PassOptions | ||
def syncWithOptions(opt: WriteablePassOptions): Unit = { | ||
opt.get[Boolean]("general", IntermediateFlag) foreach { | ||
flags += IntermediateFlag -> _ | ||
} | ||
opt.get[Boolean]("general", ProfilingFlag) foreach { | ||
flags += ProfilingFlag -> _ | ||
} | ||
opt.set("io.outdir", outDir) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the data is only one flowing one way: CLI args and the global config go through the OutputManager
, and then we use some its determinations to set pass options. So this method becomes unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. We could declare all these flags directly in the singleton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. We can consider that for a follow up. Run in by Jute and see what he thinks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This flag is proposed in the ADR for the OutputManager but wasn't
implemented in #1025, which left us without a way to enable the
generation of the profile-rules.txt file that used to always be
created (other than using the global config). The benchmarks depend on
this file being created.
make fmt-fix
(or had formatting run automatically on all files edited)