This is an archive of the discontinued LLVM Phabricator instance.

[Polly][Unfinished][NFC] Restructure the command line options
Needs ReviewPublic

Authored by jdoerfert on Oct 13 2014, 3:22 PM.

Details

Reviewers
simbuerg
dpeixott
Summary
This is an unfinished patch to show the direction I try to go,
discussion are welcome.

The changes include:
  - Unify the internal option names:      PollyXxxxxYyyyZzzz
  - Unify the command line options names: -polly-Xxxx-Yyyy-Zzzz
  - Collect all command line options in Optiohs.{h,cpp}
  - Expose all options to other LLVM projects (or passes)
  - Categorize and document the options (all in Option.h)

Diff Detail

Event Timeline

jdoerfert updated this revision to Diff 14829.Oct 13 2014, 3:22 PM
jdoerfert retitled this revision from to [Polly][Unfinished][NFC] Restructure the command line options.
jdoerfert updated this object.
jdoerfert edited the test plan for this revision. (Show Details)
jdoerfert added subscribers: Restricted Project, Unknown Object (MLST).
jdoerfert added inline comments.Oct 13 2014, 3:24 PM
lib/CodeGen/BlockGenerators.cpp
40

Can somebody explain this option to me?

simbuerg edited edge metadata.Oct 13 2014, 11:05 PM

This is exactly what I feared.
I'm not sure if I'm the only one, but I really like the definition of the command-line options where they matter: In the file where they influence something. Why would we want to extract the definitions too? Isn't it sufficient to provide the Options.h header and be done with it (+unify the naming)?

Furthermore this adds maintenance overhead: The description the end-user gets should be enough to understand what the option does. So reading the cl::desc field of a definition should suffice to understand what the option will do. Now you have to keep the comment synchronized to the description (nobody will do that ;-)).

Maybe I'm old, but the only benefits I see here are the external linkage via Options.h and the consistent naming, no need for relocating the definitions too.

In D5762#5, @simbuerg wrote:

Maybe I'm old, but the only benefits I see here are the external linkage via Options.h and the consistent naming, no need for relocating the definitions too.

I find myself more often than I like looking for command line options to control something. I want to know the command line name and the default value, however that's not that easy:

Do you know where SCEVCodegen is declared, how to enable OpenMP code generation or what option will disable runtime alias checks?

These are some of the options that have a non-local effect; thus you might not find in the command line option declaration in the files you'd expect.

If you are interested in the effect of the internal command line option variable you will still see the same as before if you only open your pass. If the option variables are namend properly you don't need to look at the command line option declaration to understand the code.

if (PollyEnableTiling)
  XXX

I do not insist on this change, if others have the same objections or you feel realy strongly about this I don't mind take that part back. But in general I still think it will clean up Polly and help us.

[btw. SCEVCodegen -> BlockGenerators.cpp, OpenMP -> CodeGeneration.cpp, RTCs -> ScopDetection.cpp]

Alright, I got a closer second look and now I think it actually looks nicer than before ;-). Thanks for explaining.

I'll wait for a thrid opinion before I proceed here.

dpeixott edited edge metadata.Oct 14 2014, 10:11 AM

This looks like a nice cleanup to me. I think centralizing the available options for polly makes sense. The major downside in my opinion is that we now have a bunch of global variables. Previously you could look at a command line option that is declared static (without external storage) and know it is only used in that file. Now we are exposing these all as global variables.

As for the implementation, maybe we should consider using an opt or options namespace inside polly for all the command line options. This would make it clear when reading the code that we are referencing a command line option and we know where to look for the definition if needed.

Something like

namespace polly {
namespace opt {
  bool DisablTiling
}}

...
if (opt::DisableTiling) 
 ...

You are already essentially namespacing the variables by adding a Polly prefix.

We should also be aware of how llvm is changing command line options to move away from using static initializers: http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-August/075886.html. I'm not sure exactly what conclusion they came to, but we don't want to come with a design that will be incompatible with where the llvm community is headed.

In D5762#9, @dpeixott wrote:

This looks like a nice cleanup to me. I think centralizing the available options for polly makes sense. The major downside in my opinion is that we now have a bunch of global variables. Previously you could look at a command line option that is declared static (without external storage) and know it is only used in that file. Now we are exposing these all as global variables.

That is actually one of the reasons I came up with this patch. I want the options to be available to other LLVM pasess/projects, thus I have to expose them somehow. While I agree that global variables are not the best solution to this problem it is/was the easiest solution to allow me to configure Polly from within another project. Do you think we should wait/look for a different solution without the globals?

As for the implementation, maybe we should consider using an opt or options namespace inside polly for all the command line options. This would make it clear when reading the code that we are referencing a command line option and we know where to look for the definition if needed.

Nice idea, I will do this.

You are already essentially namespacing the variables by adding a Polly prefix.

Indeed. Should I replace or add the opt namespace (what do you think?)

We should also be aware of how llvm is changing command line options to move away from using static initializers: http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-August/075886.html. I'm not sure exactly what conclusion they came to, but we don't want to come with a design that will be incompatible with where the llvm community is headed.

I think this is not a blocking concern right now. As soon as they come to a conclusion we will adobpt it, however if we change 10 global variables to createXXXPass arguments then or 30 shouldn't make much of a difference. In the meantime we get closer to an organized and document way of handling command line options. We can reuse the documentation and structure also for the proposed new interface:

From:
  /// @brief Scheduler Options
  ///
  ///{
  
  /// @brief LaLaLa
  bool opt::PollyOption0;

  ///}

To:
  /// @brief Create Scheduling related passes
  ///
  ///{
  
  /// @brief XXX
  ///
  /// @bparam LaLaLa
  createXXXXPass(bool opt::PollyOption0);

  ///}

Do you think the discussion on the list blocks a refactoring like this?

[I talked to Tobias; he will be fine with any decision we come up with in our discussion here]

In D5762#10, @jdoerfert wrote:
In D5762#9, @dpeixott wrote:

This looks like a nice cleanup to me. I think centralizing the available options for polly makes sense. The major downside in my opinion is that we now have a bunch of global variables. Previously you could look at a command line option that is declared static (without external storage) and know it is only used in that file. Now we are exposing these all as global variables.

That is actually one of the reasons I came up with this patch. I want the options to be available to other LLVM pasess/projects, thus I have to expose them somehow. While I agree that global variables are not the best solution to this problem it is/was the easiest solution to allow me to configure Polly from within another project. Do you think we should wait/look for a different solution without the globals?

I am ok to go with your current patch. We are already using globals in Polly, so this patch is effectively cleaning up and centralizing existing behavior. I don't see global variables used like this anywhere else in LLVM, so it is concerning from that perspective, but I don't think it needs to hold up this patch.

As for the implementation, maybe we should consider using an opt or options namespace inside polly for all the command line options. This would make it clear when reading the code that we are referencing a command line option and we know where to look for the definition if needed.

Nice idea, I will do this.

Cool. I would suggest either using opt which is nice and short, but also make me think "optimization", or knob for the namespace.

You are already essentially namespacing the variables by adding a Polly prefix.

Indeed. Should I replace or add the opt namespace (what do you think?)

I would drop the Polly prefix. From within Polly namespace it will look fine to write opt::Foo instead of opt::PollyFoo, and from outside people can use polly::opt::Foo, which I think looks nicer than polly::opt::PollyFoo.

We should also be aware of how llvm is changing command line options to move away from using static initializers: http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-August/075886.html. I'm not sure exactly what conclusion they came to, but we don't want to come with a design that will be incompatible with where the llvm community is headed.

I think this is not a blocking concern right now. As soon as they come to a conclusion we will adobpt it, however if we change 10 global variables to createXXXPass arguments then or 30 shouldn't make much of a difference. In the meantime we get closer to an organized and document way of handling command line options. We can reuse the documentation and structure also for the proposed new interface:

From:
  /// @brief Scheduler Options
  ///
  ///{
  
  /// @brief LaLaLa
  bool opt::PollyOption0;

  ///}

To:
  /// @brief Create Scheduling related passes
  ///
  ///{
  
  /// @brief XXX
  ///
  /// @bparam LaLaLa
  createXXXXPass(bool opt::PollyOption0);

  ///}

Do you think the discussion on the list blocks a refactoring like this?

No I don't think it should block the refactoring. I just brought it up in case you had some good thoughts about it. Hopefully we will be able to adapt to the change easier by having a centralized location for argument handling.

[I talked to Tobias; he will be fine with any decision we come up with in our discussion here]

Ok, sounds good.

sebpop resigned from this revision.Sep 19 2016, 1:19 PM
sebpop removed a reviewer: sebpop.
grosser edited edge metadata.Sep 30 2016, 12:18 PM

I never was excited about this change, but was OK with it to go in assuming there was agreement that the command line refactoring aimed for is preferable. This agreement had been reached, but for unknown reasons the patch was never completed. As it is now pretty old, we should probably start a new discussion in case anybody is interested in such a change.

I drop from the review to clean my queue.

grosser resigned from this revision.Sep 30 2016, 12:18 PM
grosser removed a reviewer: grosser.
include/polly/Options.h