This is an archive of the discontinued LLVM Phabricator instance.

Split fast-basictest.ll according to passes responsible for optimizations
ClosedPublic

Authored by kovdan01 on Feb 2 2022, 3:13 AM.

Details

Summary
  • add logically missing test cases;
  • add appropriate comments;
  • add appropriate TODO's.

See initial motivation in https://reviews.llvm.org/D117302

Diff Detail

Event Timeline

kovdan01 requested review of this revision.Feb 2 2022, 3:13 AM
kovdan01 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2022, 3:13 AM
spatel added a comment.Feb 2 2022, 7:41 AM

I didn't go over each test, but this seems good to me in providing better visibility about what each pass is responsible for.

For the files that still run multiple passes, I think we're trying to show that the combination of passes are cooperating to produce the expected/optimal result.
Can we move those to "llvm/test/Transforms/PhaseOrdering" and run "-O2" instead? That would show that we have the expected result from the typical optimization pipeline without needing to name specific passes. Then if something alters the pipeline and breaks the expected result, we'd know sooner.

For the files that still run multiple passes, I think we're trying to show that the combination of passes are cooperating to produce the expected/optimal result.
Can we move those to "llvm/test/Transforms/PhaseOrdering" and run "-O2" instead? That would show that we have the expected result from the typical optimization pipeline without needing to name specific passes. Then if something alters the pipeline and breaks the expected result, we'd know sooner.

Thanks for this remark, moving such tests to llvm/test/Transforms/PhaseOrdering makes sense. Regarding using -O2 instead of naming specific passes - I suppose that both options have their benefits (we want to know if something breaks both in the whole pipeline and in a specific combination of passes), so I think that we can test both variants with --check-prefix opt's option. Such approach is already taken, for example, in X86/pr52078.ll. Please let me know if you approve such approach, and if yes - I'll do that.

spatel added a comment.Feb 2 2022, 9:48 AM

For the files that still run multiple passes, I think we're trying to show that the combination of passes are cooperating to produce the expected/optimal result.
Can we move those to "llvm/test/Transforms/PhaseOrdering" and run "-O2" instead? That would show that we have the expected result from the typical optimization pipeline without needing to name specific passes. Then if something alters the pipeline and breaks the expected result, we'd know sooner.

Thanks for this remark, moving such tests to llvm/test/Transforms/PhaseOrdering makes sense. Regarding using -O2 instead of naming specific passes - I suppose that both options have their benefits (we want to know if something breaks both in the whole pipeline and in a specific combination of passes), so I think that we can test both variants with --check-prefix opt's option. Such approach is already taken, for example, in X86/pr52078.ll. Please let me know if you approve such approach, and if yes - I'll do that.

Yes, we can use >1 RUN line for more coverage. There's little extra cost with that approach.

kovdan01 updated this revision to Diff 405577.Feb 3 2022, 4:19 AM

@spatel Fixed the issue we were talking about - could you give a review on the updated patch please?

spatel added inline comments.Feb 3 2022, 5:00 AM
llvm/test/Transforms/PhaseOrdering/fast-reassociate-gvn.ll
13–14

We can reduce the number (maybe eliminate all) of the duplicated check lines by using a shared prefix. Try something like this:

; RUN: opt < %s -reassociate -gvn -S | FileCheck %s --check-prefixes=CHECK,REASSOC_AND_GVN
; RUN: opt < %s -O2 -S | FileCheck %s --check-prefixes=CHECK,O2
kovdan01 updated this revision to Diff 405619.Feb 3 2022, 6:39 AM
kovdan01 added inline comments.
llvm/test/Transforms/PhaseOrdering/fast-reassociate-gvn.ll
13–14

Thanks for that hint! Fixed.

spatel accepted this revision.Feb 3 2022, 7:00 AM

LGTM

This revision is now accepted and ready to land.Feb 3 2022, 7:00 AM
This revision was landed with ongoing or failed builds.Feb 4 2022, 1:22 AM
This revision was automatically updated to reflect the committed changes.