This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck] Catch missspelled directives.
ClosedPublic

Authored by kosarev on May 14 2022, 5:12 AM.

Diff Detail

Event Timeline

kosarev created this revision.May 14 2022, 5:12 AM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
kosarev requested review of this revision.May 14 2022, 5:12 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 14 2022, 5:12 AM

A work in progress. Contains changes that we likely want to see addressed separately.

Code change looks good to me. Are the TODO cases where the test fails if changing them?

llvm/test/FileCheck/missspelled-directive.txt
19

What about P4-COUNT_2? Is that case not really related?

Are the TODO cases where the test fails if changing them?

Yes, this and where the fix is not perfectly obvious.

llvm/test/FileCheck/missspelled-directive.txt
19

Yeah, that's intentionally left for later. At the moment I don't even know how severe the problem with COUNT_* is, need to try and see.

thopre added inline comments.May 16 2022, 4:25 AM
llvm/lib/FileCheck/FileCheck.cpp
1774–1781

Instead of introducing a new wrapper, why don't you change all the return to call a constructor method (e.g. make_check_type()) that does what this wrapper do? Then there would not be any FindCheckType that take a Misspelled parameter.

I'm also not sure about Misspelled being a check kind. It feels conceptually wrong but on the other hand I guess it makes the implementation simpler.

kosarev added inline comments.May 17 2022, 3:11 AM
llvm/lib/FileCheck/FileCheck.cpp
1774–1781

Tried that. Replacing the returned pair with a new CheckLine kind of object implementing the misspelled-related logic seems to add a lot of extra clutter such as the definition of the new structure itself, but especially all the repetitive mentions of Misspelled on every return. Feels like having it as a reference parameter works better, as we only need to alter the flag occasionally.

Regarding CheckMisspelled, now that we have CheckBadNot and CheckBadCount, this looks the usual way of propagating the information about our spelling-related concerns. Might be not the best design and may be worth looking into at some point, but at least doesn' seem to be specific to this patch?

kosarev updated this revision to Diff 429997.May 17 2022, 4:33 AM

Added MLIR fixes.

thopre added inline comments.May 17 2022, 7:41 AM
llvm/lib/FileCheck/FileCheck.cpp
1774–1781

I was thinking something along the line of:

return getRealCheckType(CHECK::CheckBadCount, Rest, Misspelled); with:

static std::pair<Check::FileCheckType, StringRef>
getRealCheckType(Check::FileCheckType CheckType, StringRef Rest, bool Misspelled) {
  if (CheckType != Check::CheckNone && Misspelled)
    return {Check::CheckMisspelled, Rest};
  return {CheckType, Rest};
}

Fair enough for CheckMisspelled, there is indeeed precedent.

kosarev added inline comments.May 17 2022, 8:00 AM
llvm/lib/FileCheck/FileCheck.cpp
1774–1781

That unfortunately wouldn't eliminate the repetitive return getRealCheckType(..., Misspelled) bits, thus adding a significant amount of clutter -- all for the sake of a single assignment where we raise the flag, while also making the code more fragile as the compiler wouldn't then be able to catch returns without calling getRealCheckType(). And if that's not enough, then the name of the function sounds like we introduce one of the most irritating kinds of concepts -- the 'real' ones. :-)

thopre added inline comments.May 18 2022, 1:26 AM
llvm/lib/FileCheck/FileCheck.cpp
1774–1781

Fair enough. LGTM for the FileCheck part then. Have you sent a message to discourse to ask test authors for help on the TODO?

kosarev updated this revision to Diff 430322.May 18 2022, 4:20 AM

Add Polly fixes.

Thanks @kosarev - CodeGen\X86 should be clean now!

RKSimon added inline comments.May 18 2022, 4:40 AM
llvm/test/DebugInfo/X86/debug-info-template-parameter.ll
36 ↗(On Diff #430322)

Should be fixed by rGf718664866ab

Tagging more people in hope to add visibility.

clang/test/CodeGen/cmse-clear-return.c
235 ↗(On Diff #430322)
llvm/test/CodeGen/AArch64/fp16-v8-instructions.ll
682–688 ↗(On Diff #430322)
llvm/test/CodeGen/AMDGPU/phi-vgpr-input-moveimm.mir
3–5 ↗(On Diff #430322)
llvm/test/CodeGen/Thumb2/thumb2-execute-only-prologue.ll
13 ↗(On Diff #430322)
llvm/test/CodeGen/WebAssembly/libcalls.ll
52–54
llvm/test/CodeGen/X86/GlobalISel/select-ext.mir
69–72 ↗(On Diff #430322)
llvm/test/DebugInfo/X86/debug-info-template-parameter.ll
33–36 ↗(On Diff #430322)
mlir/test/Conversion/MemRefToSPIRV/alloc.mlir
44 ↗(On Diff #430322)
dmgreen added inline comments.
llvm/test/CodeGen/AArch64/fp16-v8-instructions.ll
682–688 ↗(On Diff #430322)

These lines should be removed. The were accidentally left in as the file was update_llc_test_check'd

llvm/test/CodeGen/Thumb2/thumb2-execute-only-prologue.ll
13 ↗(On Diff #430322)

I think ; CHECK-NEXT: .long 4294965696 should be OK. That looks like it would match up with the code.

RKSimon added inline comments.May 18 2022, 6:57 AM
llvm/test/CodeGen/AArch64/fp16-v8-instructions.ll
682–688 ↗(On Diff #430322)
kosarev updated this revision to Diff 430392.May 18 2022, 8:34 AM

Added Flang fixes and rebased.

Thanks Simon for the quick turnaround!

Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2022, 8:34 AM
MaskRay accepted this revision.May 21 2022, 4:09 PM

LGTM. Thanks for pinging the relevant folks on individual tests. You can push the fixes yourself before landing this patch.

This revision is now accepted and ready to land.May 21 2022, 4:09 PM
kosarev updated this revision to Diff 431979.May 25 2022, 7:10 AM

Rebased on top of fixes for non-trivial cases. Going to submit this
tomorrow, if no objections.

This revision was landed with ongoing or failed builds.May 26 2022, 3:37 AM
This revision was automatically updated to reflect the committed changes.

Committing this revealed some more failures, fixed in rG8894c05b0d351f998b1836542ba791247394bd12.