Page MenuHomePhabricator

[llvm] Fix missing FileCheck directive colons
ClosedPublic

Authored by jroelofs on Apr 2 2020, 5:31 PM.

Diff Detail

Event Timeline

jroelofs created this revision.Apr 2 2020, 5:31 PM
jhenderson added inline comments.Apr 3 2020, 1:04 AM
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)?

jroelofs marked 4 inline comments as done.Apr 3 2020, 7:49 AM

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.

tra added a subscriber: tra.Apr 3 2020, 9:24 AM

LGTM for NVPTX tests.

jhenderson accepted this revision.Apr 6 2020, 12:37 AM

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.

This revision is now accepted and ready to land.Apr 6 2020, 12:37 AM

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.

Those got fixed by Simon in 396b1ee0e0b0c80484095a33daed7b53e701b090

jroelofs closed this revision.Apr 6 2020, 9:02 AM

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.