Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
@aeubanks I think you previously suggested adding an AMDGPU RUN line to llvm/test/Other/new-pm-defaults.ll instead of doing this?
I'm somewhat against covering all targets in a single test like this. I think it just makes it more difficult to maintain. e.g. I hate touching tests like clang/test/Preprocessor/predefined-arch-macros.c, there's no real value shared by the common dummy code and now you have to sort out which run line failed
| llvm/test/CodeGen/AMDGPU/opt-pipeline-newpm.ll | ||
|---|---|---|
| 531 | The generic test does have a loop, is that relevant to testing loop passes? | |
Please at least put this in llvm/test/Other next to the other pipeline tests. These tests are incredibly annoying, so lets at least avoid spreading them out across different places, if we really, really do need to have target-specific variants of them.
do we really need to test every pass when it's almost identical to the existing tests? can we just test that the specific passes that AMDGPU adds are in the pipeline?
I think, then, we will not be able to capture changes required in pipeline when someone updates AMDGPU Pass which depends on non-target specific pass. For example, in D153349, the AMDGPUAtomicOptimizer uses dom-tree but updating the dom-tree in the pass and preserving the analysis did not ask for updating the any of the pipeline tests.
Unfortunately, NewPM pipeline tests will not help you find missing pass invalidations, because it depends on on the specific IR and whether the pass ended up changing something or not. The pipeline tests are generally designed to avoid any invalidations (that are not part of the pass pipeline itself).
I don't think there is any good way to find NewPM invalidation issues right now, apart from directly tracking compile-time.
you could check Invalidating analysis: DominatorTree with some IR that previously invalidated dom-tree and now doesn't for your pass, but as nikic said, ultimately that's for compile time reasons and you should have a AMDGPU compile time tracker
checking that the AMDGPU-specific passes are in the pipeline is fine, but I don't want this test to duplicate testing the rest of the pipeline
The generic test does have a loop, is that relevant to testing loop passes?