This is an archive of the discontinued LLVM Phabricator instance.

[LV] Re-generate check lines of some fragile tests
ClosedPublic

Authored by mdchen on Jul 5 2021, 9:52 AM.

Details

Summary

Update some fragile tests related to D104148 with the latest update_test_checks.py to make reviewers' life easier.

Diff Detail

Event Timeline

mdchen created this revision.Jul 5 2021, 9:52 AM
mdchen requested review of this revision.Jul 5 2021, 9:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2021, 9:52 AM
fhahn added inline comments.Jul 5 2021, 9:58 AM
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.

mdchen added inline comments.Jul 5 2021, 7:35 PM
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.

mdchen updated this revision to Diff 356940.Jul 7 2021, 6:56 AM

Updated.

fhahn accepted this revision.Jul 8 2021, 7:37 AM

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.

This revision is now accepted and ready to land.Jul 8 2021, 7:37 AM
mdchen updated this revision to Diff 357521.Jul 9 2021, 8:37 AM
mdchen retitled this revision from [NFC] Update some fragile tests to [LV] Re-generate check lines of some fragile tests.

Hi @fhahn , to address your comments in D104148, more tests need to be updated here, please take another look, thanks very much!

mdchen added a comment.Jul 9 2021, 8:57 AM

Ops... invariant-store-vectorization-2.ll is not in the right status, will update it soon.

mdchen updated this revision to Diff 357540.Jul 9 2021, 9:23 AM

Correct invariant-store-vectorization-2.ll check lines(the previous version is generated with a version with D104148 code).

fhahn added inline comments.Jul 10 2021, 6:33 AM
llvm/test/Transforms/LoopVectorize/invariant-store-vectorization-2.ll
2

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

there's no test above?

llvm/test/Transforms/LoopVectorize/invariant-store-vectorization.ll
287

Why rename those?

mdchen added inline comments.Jul 10 2021, 8:32 AM
llvm/test/Transforms/LoopVectorize/invariant-store-vectorization-2.ll
2

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

Ahh, I should have looked into the comments carefully. Will rephrase them.

llvm/test/Transforms/LoopVectorize/invariant-store-vectorization.ll
287

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.

mdchen updated this revision to Diff 357922.Jul 12 2021, 6:22 AM
fhahn added a comment.Jul 12 2021, 1:20 PM

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

Alright, if it is just from the existing test that's fine, even though not ideal.

15

it still says 'of the above test`?

llvm/test/Transforms/LoopVectorize/invariant-store-vectorization.ll
287

sounds good to me, thanks!

mdchen updated this revision to Diff 358120.Jul 12 2021, 7:20 PM

Update the comments in invariant-store-vectorization-2.ll.

@fhahn Thank you very much.

llvm/test/Transforms/LoopVectorize/X86/invariant-store-vectorization.ll
55

In D104148 this file needs an update, so I re-generated it to exclude some noise.

This revision was landed with ongoing or failed builds.Jul 19 2021, 4:39 AM
This revision was automatically updated to reflect the committed changes.