[DFAJumpThreading] Enable the pass by default

[UsmanNadeem]

https://discourse.llvm.org/t/rfc-enable-dfa-jumpthreading-pass-by-default/77231

[nikic]

Enabling this pass by default currently causes a timeout in the stage 2 build: http://llvm-compile-time-tracker.com/show_error.php?commit=f3418c749f6060a5610020ef53a5d78e222be455

As such I can't provide up to date compile-time numbers, but the last time I tried there were unacceptably large regressions, and I don't think anything has changed since (http://llvm-compile-time-tracker.com/compare.php?from=387c1573f89117687f4b964ae3a90ea7c91a4f90&to=2a66b841e34f385331226d2b4f89fffd1840cda1&stat=instructions:u).

This pass is not production ready.

[UsmanNadeem]

Enabling this pass by default currently causes a timeout in the stage 2 build: http://llvm-compile-time-tracker.com/show_error.php?commit=f3418c749f6060a5610020ef53a5d78e222be455

As such I can't provide up to date compile-time numbers, but the last time I tried there were unacceptably large regressions, and I don't think anything has changed since (http://llvm-compile-time-tracker.com/compare.php?from=387c1573f89117687f4b964ae3a90ea7c91a4f90&to=2a66b841e34f385331226d2b4f89fffd1840cda1&stat=instructions:u).

This pass is not production ready.

@nikic What would be acceptable compile-time numbers?

[nikic]

@UsmanNadeem Given what the pass does, you should aim for 0.1% first-order impact. (IIRC the cost of the original implementation was in that ballpark.)

[XChy]

The performance improvement brought by DFAJumpThreading looks fairly good to me. But it's uncertain whether we test this pass enough for correctness, especially with just few maintainance after the initial patch of DFAJumpThreading. To add it into the pipeline, I believe we need more correctness proofs apart from internal unit tests.

[UsmanNadeem]

The performance improvement brought by DFAJumpThreading looks fairly good to me. But it's uncertain whether we test this pass enough for correctness, especially with just few maintainance after the initial patch of DFAJumpThreading. To add it into the pipeline, I believe we need more correctness proofs apart from internal unit tests.

I believe that enabling it by default will enable more testing, and thus uncover more bugs. We have plenty of time till the next release to fix them.

[UsmanNadeem]

@UsmanNadeem Given what the pass does, you should aim for 0.1% first-order impact. (IIRC the cost of the original implementation was in that ballpark.)

After the recent patches to fix the compile time, the costs now look reasonable: https://llvm-compile-time-tracker.com/compare.php?from=2903df02fb3c057849aaa796a91289b01950a5f0&to=110e07154887a0c0f8705a7a3ffb2d25aa59f94f&stat=instructions:u

stage1-O3: (+0.08%)
stage1-ReleaseLTO-g: (+0.04%)
stage1-ReleaseThinLTO: (+0.15%)
clang build: (+0.05%)

[nikic]

Thanks, the new compile-time numbers look a lot better!

While this mostly resolves the compile-time issues for average cases, I am still concerned about the behavior of the pass in pathological cases, in particular the discussion starting at #85015 (comment). We need to make sure that even if all the early-exit conditions don't trigger, we never inspect an unreasonably large number of paths.

[djtodoro]

Hi, what is the status of this PR? What are the next steps?

[djtodoro]

We need to make sure that even if all the early-exit conditions don't trigger, we never inspect an unreasonably large number of paths.

Could we implement a threshold to limit the number of paths inspected? This could be set as a backend option.

[UsmanNadeem]

Hi, what is the status of this PR? What are the next steps?

I have merged #96127 which changes the algorithm to reduce the number of paths inspected and also adds more limits to reduce compile time. There are some assertions triggered after the patch which I am planning to look into soon. Here is the open issue: #106083

[djtodoro]

Hi, what is the status of this PR? What are the next steps?

I have merged #96127 which changes the algorithm to reduce the number of paths inspected and also adds more limits to reduce compile time. There are some assertions triggered after the patch which I am planning to look into soon. Here is the open issue: #106083

Since #109511 got merged, we can proceed with this one, right?

[UsmanNadeem]

Hi, what is the status of this PR? What are the next steps?

I have merged #96127 which changes the algorithm to reduce the number of paths inspected and also adds more limits to reduce compile time. There are some assertions triggered after the patch which I am planning to look into soon. Here is the open issue: #106083

Since #109511 got merged, we can proceed with this one, right?

@nikic ping!

[nikic]

Can you please rebase the PR?

[nikic]

New compile-time: https://llvm-compile-time-tracker.com/compare.php?from=29b92d07746fac26cd64c914bc9c5c3833974f6d&to=bcc333192cfadbe63154294b8606fba53247c7fd&stat=instructions:u

The results on CTMark look reasonable. Clang has two big regressions:

lib/DebugInfo/PDB/CMakeFiles/LLVMDebugInfoPDB.dir/Native/NativeInlineSiteSymbol.cpp.o 	5979M 	7853M (+31.34%)
tools/clang/lib/AST/CMakeFiles/obj.clangAST.dir/ByteCode/Interp.cpp.o 	39908M 	43794M (+9.74%)

[nikic]

More results from llvm-opt-benchmark:

Top 5 regressions:
  openssl/openssl-bin-smime.ll 475446451 -> 1171329774 +146.36%
  php/php_http_parser.ll 757649325 -> 1845439220 +143.57%
  hyperscan/Parser.cpp.ll 5461814098 -> 7624209225 +39.59%
  ruby/gb18030.ll 119054238 -> 161772306 +35.88%
  jq/gb18030.ll 123053668 -> 166193537 +35.06%