Update some fragile tests related to D104148 with the latest update_test_checks.py to make reviewers' life easier.
Details
Diff Detail
Event Timeline
llvm/test/Transforms/LoopVectorize/pr47343-expander-lcssa-after-cfg-update.ll | ||
---|---|---|
75 | remove whitespace? | |
llvm/test/Transforms/LoopVersioningLICM/loopversioningLICM1.ll | ||
11 ↗ | (On Diff #356520) | As we only check a single basic block, it is probably best to keep checking the hard-coded basic block names, because regex matched basic block names are only meaningful if the whole CFG is checked. |
llvm/test/Transforms/LoopVersioningLICM/loopversioningLICM2.ll | ||
11 ↗ | (On Diff #356520) | I think in D104148, the check here is causing the problem. Probably best to keep checking the hard-coded basic block names, because regex matched basic block names are only meaningful if the whole CFG is checked. |
llvm/test/Transforms/LoopVectorize/pr47343-expander-lcssa-after-cfg-update.ll | ||
---|---|---|
75 | Thanks, will remove them. | |
llvm/test/Transforms/LoopVersioningLICM/loopversioningLICM1.ll | ||
11 ↗ | (On Diff #356520) | But for variables that we don't really care about, like bb names here, wouldn't it be better to have them matched by .*, with or without names? Otherwise, we may have to manually modify them again for an unrelated change in the future. Also, it may cause some confusion about if the CFG has changed. |
LGTM, thanks.
It would probably be good to update the title of the commit to be a bit more specific, e.g. something like [LV] Re-generate check lines of some fragile tests.
Ops... invariant-store-vectorization-2.ll is not in the right status, will update it soon.
Correct invariant-store-vectorization-2.ll check lines(the previous version is generated with a version with D104148 code).
llvm/test/Transforms/LoopVectorize/invariant-store-vectorization-2.ll | ||
---|---|---|
2 ↗ | (On Diff #357540) | The test should ideally not depend on licm. Can you update it so it does not require either LICM run? Also, it seems this contains similar comments to llvm/test/Transforms/LoopVectorize/invariant-store-vectorization.ll. what's the difference? |
15 ↗ | (On Diff #357540) | there's no test above? |
llvm/test/Transforms/LoopVectorize/invariant-store-vectorization.ll | ||
586 | Why rename those? |
llvm/test/Transforms/LoopVectorize/invariant-store-vectorization-2.ll | ||
---|---|---|
2 ↗ | (On Diff #357540) | This test file used to be part of llvm/test/Transforms/LoopVectorize/invariant-store-vectorization.ll. I moved these tests into a separate file because they're auto-generated tests and mix them with hard-coded checks is hard to maintain(mainly because of the numbered metadata). |
15 ↗ | (On Diff #357540) | Ahh, I should have looked into the comments carefully. Will rephrase them. |
llvm/test/Transforms/LoopVectorize/invariant-store-vectorization.ll | ||
586 | Because tmp prefix might conflict with the auto-generated check line variables and update_test_checks.py throws warnings for them. I suppose these tmp prefixed names are generated by the old version of opt -instnamer, and the latest version use i instead. |
Thanks, still LGTM with the remaining suggestions.
llvm/test/Transforms/LoopVectorize/X86/invariant-store-vectorization.ll | ||
---|---|---|
55 | no changes other than the loop check in this file, so no need to include them? | |
llvm/test/Transforms/LoopVectorize/invariant-store-vectorization-2.ll | ||
2 ↗ | (On Diff #357540) | Alright, if it is just from the existing test that's fine, even though not ideal. |
15 ↗ | (On Diff #357540) | it still says 'of the above test`? |
llvm/test/Transforms/LoopVectorize/invariant-store-vectorization.ll | ||
586 | sounds good to me, thanks! |
no changes other than the loop check in this file, so no need to include them?