[llvm] ConstraintElim: add dry-run routine to fail early (PR #99670)

[Ramkumar Ramachandra via llvm-commits]

artagnon wrote:

> I had a chance to check SPEC2017 and there are a few cases where fewer conditions are simplified (with -O3)
> 
> ```
> Program                                       constraint-elimination.NumCondsRemoved
>                                               base                                   patch   diff
> External/S...NT2017rate/502.gcc_r/502.gcc_r   1123.00                                1098.00  -2.2%
> External/S...rate/510.parest_r/510.parest_r    273.00                                 262.00  -4.0%
> External/S...00.perlbench_r/500.perlbench_r     97.00                                  88.00  -9.3%
> External/S...te/538.imagick_r/538.imagick_r     30.00                                  24.00 -20.0%
> External/S...te/526.blender_r/526.blender_r    599.00                                 344.00 -42.6%
> ```
> 
> In Sqlite from the test suite, there also are a few cases where we have fewer simplifications (but no binary changes probably due to them being simplified later).
> 
> I am wondering if we should aim for a check like below, to surface more cases where the behavior changes (with that change, I also tried bootstrapping Clang but couldn't get very far)
> 
> ```
> diff --git a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
> index 40d117d49489..916f90c1fa0f 100644
> --- a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
> +++ b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
> @@ -1898,8 +1898,8 @@ static bool eliminateConstraints(Function &F, DominatorTree &DT, LoopInfo &LI,
> 
>    // Fail early if estimates exceed limits. Row estimate could be off by up to
>    // 40%.
> -  if (EstimatedRows > 1.4 * MaxRows || EstimatedColumns > MaxColumns)
> -    return false;
> +  // if (EstimatedRows > 1.4 * MaxRows || EstimatedColumns > MaxColumns)
> +  //  return false;
> 
>    SmallVector<Value *> FunctionArgs;
>    for (Value &Arg : F.args())
> @@ -2058,6 +2058,9 @@ static bool eliminateConstraints(Function &F, DominatorTree &DT, LoopInfo &LI,
> 
>    for (Instruction *I : ToRemove)
>      I->eraseFromParent();
> +
> +  assert(!Changed ||
> +         (EstimatedRows <= 1.4 * MaxRows && EstimatedColumns <= MaxColumns));
>    return Changed;
>  }
> ```

My gut reaction is that this diff will not be helpful: we will get a lot of asserts in the case of binary changes, and these asserts will not have information to meaningfully update the estimates. The dry-run estimation can't be used for a logically precise condition: it is a cheap estimation that "works in practice". I wish we had some kind of formula for the upper-bound we could just lift out from a paper (as is the case for many algorithms like FM), but this is not possible for ConstraintElimination.

> Presumably some of those issues would go away with a larger default for MaxColumns?

My intuition from writing the dry-run estimation routine is that the 1.4 factor is the more problematic issue, and not MaxColumns. We should bump the factor to 1.6, and try again: the reason I used 1.4 is because I wanted a fine upper bound so we don't lose the benefits of the patch; 1.6 could work just as well, but a factor that's too high might not yield such great overall results. I _think_ the row-estimation is off by a constant factor (modulo minor error), so multiplying a constant factor should be _possible_.

I've observed that the column-estimation is pretty accurate, although it is possible that MaxColumns would need to be bumped up to 64 if the row-estimation times the constant factor is correct.

https://github.com/llvm/llvm-project/pull/99670