[PATCH] [Polly][Unfinished][NFC] Restructure the command line options

David Peixotto dpeixott at codeaurora.org
Wed Oct 15 18:52:23 PDT 2014


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

http://reviews.llvm.org/D5762






More information about the llvm-commits mailing list