This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck] Disallow unused prefixes in llvm/test/Analysis
ClosedPublic

Authored by mtrofin on Nov 11 2020, 9:15 AM.

Details

Summary

This is achieved through a substitution of FileCheck in lit.cfg.py,
where we explicitly set -allow-unused-prefixes to false.

We also introduce a %FileCheckWithUnusedPrefixes% substitution that can
be used in those cases where we want to allow unused prefixes, even if
the folder policy is to disallow them.

Diff Detail

Event Timeline

mtrofin created this revision.Nov 11 2020, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2020, 9:15 AM
mtrofin requested review of this revision.Nov 11 2020, 9:15 AM
MaskRay added inline comments.
llvm/test/lit.cfg.py
87 ↗(On Diff #304541)

Does ('FileCheck', 'FileCheck --allow-unused-prefixes=true') work?

You may take a look at some tools like opt,llc,llvm-mc,etc. They expand to the absolute paths. It is possible that FileCheck can use the same trick, but expand with another argument, so that you don't have to update the tests.

mtrofin added inline comments.Nov 11 2020, 10:31 AM
llvm/test/lit.cfg.py
87 ↗(On Diff #304541)

The intent is to have a way to set --allow-unused-prefixes=false under whole directories, and offer a "way out" for the files where we want to allow it. This way, we can "seal" to the desired behavior whole directories after they are fixed.

This patch does that for llvm/test/Analysis.

We still don't want to blanket-allow the undesirable behavior for entire directories.

Looks okay, but I'm not really up to speed on the implications of lit substitutions, so it would be better for someone else to review if possible. In the ideal world, we'll revert this and any similar patches and just use the option explicitly, once we've fixed all the tests and changed the default to reduce obfuscation, in my opinion, but I'm not sure it's a big issue.

llvm/test/Analysis/lit.local.cfg
6
10

Nit: need new line at EOF.

So I'm clear - the final patch is just the lit.local.cfg changes? AFAICT the changes to arith-* are just examples that aren't actually needed

Looks okay, but I'm not really up to speed on the implications of lit substitutions, so it would be better for someone else to review if possible. In the ideal world, we'll revert this and any similar patches and just use the option explicitly, once we've fixed all the tests and changed the default to reduce obfuscation, in my opinion, but I'm not sure it's a big issue.

Right - when it's all done, meaning either prefixes are removed or test maintainers believe unused ones are intentional, we can:

  • flip the default
  • undo this change
  • s/%FileCheckWithUnusedPrefixes%/--allow-unused-prefixes=true/g

So I'm clear - the final patch is just the lit.local.cfg changes? AFAICT the changes to arith-* are just examples that aren't actually needed

Not sure what you mean by 'final' - this is a patch to "stop the bleeding" in test/Analysis.

Happy to address the 2 arith-* cases if that's the case - I'd still leave the %FileCheckWithUnusedPrefixes% so we can use them elsewhere - i.e. for scripted fixups (with a "FIXME: is this intended?") for folders that don't appear to be maintained, or for folders where this is the intent (without the FIXME there, of course).

Sorry - I hadn't realised that some of the x86 costmodel tests were using the --allow-unused-prefixes flag - I'll see if I can drop them.

mtrofin updated this revision to Diff 304980.Nov 12 2020, 3:12 PM

missing newline

The arith-fp.ll + arith-fma.ll changes shouldn't be needed after rGe11195d0a93239

mtrofin updated this revision to Diff 305778.Nov 17 2020, 6:55 AM
mtrofin marked 3 inline comments as done.

Addressed comments

llvm/test/Analysis/lit.local.cfg
10

done

gentle reminder - is this OK to land now? Thanks!

RKSimon added inline comments.Nov 18 2020, 9:04 AM
llvm/test/lit.cfg.py
87 ↗(On Diff #304541)

Do we still need this?

mtrofin marked an inline comment as done.Nov 18 2020, 9:32 AM
mtrofin added inline comments.
llvm/test/lit.cfg.py
87 ↗(On Diff #304541)

Not for this change. It may help with other changes where we want to both introduce the desirable behavior for whole directories, and piecemeal opt out tests. But I can remove it from here and reintroduce it when we hit the problem - wdyt?

(for completeness, as previously discussed, once all tests are clean, we then remove the local.lit.cfg changes, and would replace any %FileCheckWithUnusedPrefixes% with the expanded FileCheck --allow-unused-prefixes)

RKSimon accepted this revision.Nov 19 2020, 2:29 AM

LGTM, cheers - please drop the %FileCheckWithUnusedPrefixes% change until/if we actually need it.

This revision is now accepted and ready to land.Nov 19 2020, 2:29 AM
mtrofin updated this revision to Diff 306414.Nov 19 2020, 7:56 AM
mtrofin marked an inline comment as done.

Scoped changes to just llvm/test/Analysis

This revision was landed with ongoing or failed builds.Nov 19 2020, 7:57 AM
This revision was automatically updated to reflect the committed changes.