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

[smeenai]

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.

[xbolva00]

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

[enrico271]

@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]

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]

Add test

[smeenai]

Added test, as suggested by @jdoerfert on IRC.

[xbolva00]

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

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

[sanjoy]

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.

[smeenai]

In D67512#1674846, @sanjoy wrote:

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!

[enrico271]

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

llvm/lib/Analysis/ScalarEvolution.cpp

STATISTIC(NumTripCountsComputed,
           "Number of loops with predictable loop counts");
 STATISTIC(NumTripCountsNotComputed,
           "Number of loops without predictable loop counts");
 STATISTIC(NumBruteForceTripCountsComputed,
           "Number of loops with trip counts computed by force");
 
 static cl::opt<unsigned>
 MaxBruteForceIterations("scalar-evolution-max-iterations", cl::ReallyHidden,
+                        cl::ZeroOrMore,
                         cl::desc("Maximum number of iterations SCEV will "
                                  "symbolically execute a constant "
                                  "derived loop"),
                         cl::init(100));
 
 // FIXME: Enable this with EXPENSIVE_CHECKS when the test suite is clean.
 static cl::opt<bool> VerifySCEV(
     "verify-scev", cl::Hidden,

llvm/test/Analysis/ScalarEvolution/multiple-max-iterations.ll

+; Ensure we can pass -scalar-evolution-max-iterations multiple times
+; RUN: opt -S -scalar-evolution -scalar-evolution-max-iterations=42 -scalar-evolution-max-iterations=42 < %s