Page MenuHomePhabricator

[Analysis] Allow -scalar-evolution-max-iterations more than once
ClosedPublic

Authored by smeenai on Sep 12 2019, 12:08 PM.

Details

Summary

At present, -scalar-evolution-max-iterations is a cl::Optional
option, which means it demands to be passed exactly zero or one times.
Our build system makes it pretty tricky to guarantee this. We often
accidentally pass the flag more than once (but always with the same
value) which results in an error, after which compilation fails:

clang (LLVM option parsing): for the -scalar-evolution-max-iterations option: may only occur zero or one times!

It seems reasonable to allow -scalar-evolution-max-iterations to be
passed more than once. Quoting the documentation:

The cl::ZeroOrMore modifier ... indicates that your program will allow the option to be specified zero or more times.
...
If an option is specified multiple times for an option of the cl::opt class, only the last value will be retained.

Event Timeline

enrico271 created this revision.Sep 12 2019, 12:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 12 2019, 12:08 PM
enrico271 edited the summary of this revision. (Show Details)Sep 12 2019, 12:14 PM
enrico271 added a reviewer: danielcdh.

I dont like this. You should fix your build system instead of introducing workarounds to LLVM.

@xbolva00 Thanks for your input!
The idea is similar to this change: https://reviews.llvm.org/D34219. I don't view it as a workaround, but rather it increases flexibility in the flag usage. The expected behavior is already well documented:

The cl::ZeroOrMore modifier ... indicates that your program will allow the option to be specified zero or more times.
If an option is specified multiple times for an option of the cl::opt class, only the last value will be retained.

I see a bunch of other flags use cl::ZeroOrMore as well so I believe this is no different.

lebedev.ri resigned from this revision.Sep 13 2019, 9:31 AM

While clearly there are bugs in whatever build system that causes this, the change feels kind-of risk-less to me, but i have no strong opinion here.

smeenai commandeered this revision.Sep 17 2019, 10:41 AM
smeenai added a reviewer: enrico271.
smeenai updated this revision to Diff 220581.Sep 17 2019, 3:25 PM
smeenai retitled this revision from Allow -scalar-evolution-max-iterations more than once to [Analysis] Allow -scalar-evolution-max-iterations more than once.
smeenai edited the summary of this revision. (Show Details)
smeenai added a reviewer: jdoerfert.

Add test

Added test, as suggested by @jdoerfert on IRC.

xbolva00 accepted this revision.Sep 17 2019, 4:01 PM

Since similar patch was accepted, I am ok with this. +1 for test.

Wait a day or so if anybody is agaist this patch.

This revision is now accepted and ready to land.Sep 17 2019, 4:01 PM
sanjoy accepted this revision.Sep 18 2019, 8:19 PM

This patch individually is fine, but why are you setting scalar-evolution-max-iterations in the first place? It is a debug option that end-users should ideally never have to change.

This patch individually is fine, but why are you setting scalar-evolution-max-iterations in the first place? It is a debug option that end-users should ideally never have to change.

I'll let @enrico271 answer that one. Thanks for the review!

scalar-evolution-max-iterations flag allows us to override the 100 iterations limit for constant derived loop, which we need for optimization, as we have many constant derived loops with > 100 iterations

This revision was automatically updated to reflect the committed changes.