Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/test/CodeGen/PowerPC/spe.ll | ||
---|---|---|
46 | I'm not sure I follow why it's safe to remove the lines from this test? | |
llvm/test/CodeGen/PowerPC/umulo-128-legalisation-lowering.ll | ||
154 | Are you sure this line should be removed? | |
llvm/test/CodeGen/X86/GlobalISel/irtranslator-callingconv.ll | ||
318 | Are you sure this line should be removed? | |
llvm/test/CodeGen/X86/avx-vzeroupper.ll | ||
86 | Any thoughts on why this line is here, and why it is safe to remove (i.e. instead of fixing)? |
I suppose I should have separated out the trivial ones from these that took a bit more reasoning to confirm.
llvm/test/CodeGen/PowerPC/spe.ll | ||
---|---|---|
46 | Have a look at the patch that added them in the first place: d52990c71b6f83c9beeba4efc9103412e0416ba9 They can't have been correct then, since PPC doesn't use @s as a label prefix, but they were pretty clearly intended to mark the beginning of the function. Later on in a51c61ea332f89dfbb9f3b3498c37b2efc99e13b, the test got reworked with utils/update_llc_test_checks.py, which didn't know to delete them. | |
llvm/test/CodeGen/PowerPC/umulo-128-legalisation-lowering.ll | ||
154 | That label can't appear twice in the ASM, and it's already matched above on line 36. | |
llvm/test/CodeGen/X86/GlobalISel/irtranslator-callingconv.ll | ||
318 | This one came from a8ba572dcfdb969fe1bb3381663298488891f838, and the subsequent rewrite in 347921a28157daf87acfcb31ce4787a46bd7ad12 is what left it in the wrong place breaking it further. | |
llvm/test/CodeGen/X86/avx-vzeroupper.ll | ||
86 | This one wasn't correct when it got added in b2b6a54f847f33f821f41e3e82bf3b86e08817a0, as that block label was already matched by another check a few lines earlier in the test. |
I'm not sure if it was intended, but the addition of colons in the original version of the patch to llvm/test/Transforms/LoopStrengthReduce/X86/lsr-insns-2.ll seem to have disappeared in the rebase. Otherwise, looks good to me.
Hi Jonathan,
If you included Differential Revision: https://reviews.llvm.org/D77352 in the git description, the differential would be closed automatically when the commit landed in master.
Many people agree that the commit description should include Reviewed-By: (not a strict rule). You can find plenty of examples in the history.
I'm not sure I follow why it's safe to remove the lines from this test?