search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.9k stars 11.51k forks source link

Should we remove unnecessary uses of -verify-machineinstrs in test/CodeGen? #102092

Open lukel97 opened 1 month ago

lukel97 commented 1 month ago

Creating this to continue the discussion in https://github.com/llvm/llvm-project/pull/100992#discussion_r1694981952, although I'm not strongly opinionated on the issue.

Across all the different targets in the CodeGen tests there are 17751 RUN lines that pass -verify-machineinstr. At least on RISC-V, a lot of these are just added by habit and aren't strictly necessary.

A quick test on my machine shows removing -verify-machineinstr reduces the time to run check-llvm-codegen-riscv (RISC-V was the only target I happened to have enabled) from ~14.44s to ~11.14s, which is about a 22% speedup.

We could investigate removing some of these uses and instead rely on the EXPENSIVE_CHECKS buildbots.

arsenm commented 1 month ago

I do think we would be better off trimming out cases, and rely on EXPENSIVE_CHECKS builds to catch the rare cases. Exceptions may apply for new tests checking a verifier error. If we're just going to add it to every test anyway, -verify-machineinstrs could just be the default and save characters in every run line.

A related issue is for MIR tests, llc has a bad default. It always runs the machine verifier to verify the input, but you still need -verify-machineinstrs to run the verifier on the final output (as well as any intermediate passes). It would be a more reasonable default to run the post verifier, with opt-in run intermediate passes. It would also be nice if we had a way to disable the post verifier, like how opt has -disable-verify for IR.

RKSimon commented 1 month ago

It would help if EXPENSIVE_CHECKS build failures were a little more prominent - their buildbot fails do tend to be ignored/lost in the noise.

I'm assuming its out of the question of getting some EXPENSIVE_CHECKS coverage added to the PR CI checks.

lukel97 commented 4 weeks ago

The EXPENSIVE_CHECKS buildbot seems to finish running in around 5 minutes on a machine with 96 cores. That's building llvm and lld with ccache and then running ninja check-all. I've no idea what hardware the PR CI runs on, but the fast feedback cycle would be nice.