Page MenuHomePhabricator

FileCheck [11/12]: Add matching constraint specification
ClosedPublic

Authored by thopre on Apr 7 2019, 4:23 PM.

Details

Summary

This patch is part of a patch series to add support for FileCheck
numeric expressions. This specific patch adds support for specifying the
matching constraint for a numeric expression, ie. how the value being
matched should relate to the numeric expression.

This commit only adds the equality constraint where the numeric value
matched must be equal to the numeric expression. It is the default
matching constraint used when not specified. It is added to provision
other matching constraint (e.g. inequality relations).

Copyright:

  • Linaro (changes up to diff 183612 of revision D55940)
  • GraphCore (changes in later versions of revision D55940 and in new revision created off D55940)

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
thopre updated this revision to Diff 198098.May 3 2019, 2:53 PM

Rebase on latest changes in unittests

thopre updated this revision to Diff 198447.May 7 2019, 5:35 AM

Rebase on top of latest changes

thopre updated this revision to Diff 198552.May 7 2019, 3:31 PM

Rebase on top of latest changes.

thopre updated this revision to Diff 198649.May 8 2019, 6:53 AM

Rebase on top of latest changes

thopre updated this revision to Diff 198954.May 9 2019, 4:54 PM

Rebase on top of latest changes

thopre updated this revision to Diff 199142.May 11 2019, 7:16 AM

Rebase on top of latest changes

thopre updated this revision to Diff 199240.May 13 2019, 3:58 AM

Rebase on top of latest changes

thopre updated this revision to Diff 199400.May 14 2019, 4:08 AM

Rebase on top of latest changes

rnk removed a reviewer: rnk.May 14 2019, 11:28 AM
thopre updated this revision to Diff 200477.May 21 2019, 5:56 AM

rebase on top of latest changes

thopre updated this revision to Diff 200554.May 21 2019, 11:15 AM

rebase on top of latest changes

thopre updated this revision to Diff 201986.May 29 2019, 10:53 AM

Rebase on top of latest changes in patch series

thopre updated this revision to Diff 203213.Jun 5 2019, 11:21 AM

Rebase on top of latest changes in the series

thopre updated this revision to Diff 205684.Jun 19 2019, 3:22 PM

Rebase on top of latest changes in patch series

thopre updated this revision to Diff 205766.Jun 20 2019, 2:45 AM

Rebase on latest changes in patch series

thopre planned changes to this revision.Dec 11 2019, 6:15 AM

Need to add unit tests

thopre planned changes to this revision.Jan 8 2020, 7:27 AM

Need to add unit tests

Let me know once this next step is ready for review!

llvm/test/FileCheck/numeric-expression.txt
452

You probably need a test case here with an explicit constraint whilst also defining a new variable. Probably also want some tests for the contraint not being an exact match but rather matching an expression, something like [[#VAR2:==VAR+1]]

Let me know once this next step is ready for review!

I have a local patch with extra unit tests but it made me realize there is a bug in the "invalid matching constraint" check: if there's something else than "==" then it'll interpret it as an operand instead. I need to rework the approach. I don't think I'll have time today, maybe Monday.

thopre updated this revision to Diff 269252.Jun 8 2020, 8:36 AM
  • Rebase
  • Add unit tests
  • Fix logic to detect invalid constraint
thopre updated this revision to Diff 269256.Jun 8 2020, 8:45 AM
thopre marked an inline comment as done.

Address review comments

thopre updated this revision to Diff 269257.Jun 8 2020, 8:46 AM

Move negative testing just after positive testing

Harbormaster failed remote builds in B59498: Diff 269256!
Harbormaster failed remote builds in B59499: Diff 269257!
jhenderson added inline comments.Jun 9 2020, 12:34 AM
llvm/docs/CommandGuide/FileCheck.rst
687–688

Perhaps worth clarifying that an empty constraint means any value is accepted.

llvm/lib/Support/FileCheck.cpp
714

I think I'd change this variable (also in the function it's used in) to be HasValidConstraint and flip the logic, i.e. HasValidConstraint is false by default, but set to true if there is a recognised constraint.

724

The term "matching" both here and in the comment above is a little bit overloaded, since I immediately thought "what is the constraint matching" when in fact it's the expression that is matching something. I'd just delete the term "matching" from all cases referring to the constraint, including the docs. I think "constraint" on its own is sufficient.

Also "Empty" -> "empty"

llvm/lib/Support/FileCheckImpl.h
731–736

\ -> /

indicate -> indicates

Obviously this comment will need updating when you switch to HasValidConstraint.

I think you can also simplify any "Parameter \p" instances in this comment to simply "\p", since it's obvious from the context what they are referring to (\p is for parameters).

llvm/test/FileCheck/numeric-expression.txt
158

Nit: missing full stop.

166

It would probably be sensible to confirm using UNSI2 works as expected, with an extra check.

I might be missing it, but do you have any test cases where an explicit constraint (as opposed to an implicit one) doesn't match?

169

Doesn't this need a ProtectFileCheckOutput line?

llvm/unittests/Support/FileCheckTest.cpp
959

Up to you, but since we'd probably want to support >= at some point in the future, I think it might be best for this to pick something that is definitely not a valid constraint, e.g. +=.

thopre updated this revision to Diff 269696.Jun 9 2020, 4:20 PM
thopre marked 10 inline comments as done.

Address review comments

llvm/docs/CommandGuide/FileCheck.rst
687–688

I don't understand what you mean. As explained here an empty constraint is the same as the == constraint and means an exact match, as in #FOO with FOO equal to 10 will only match 10. Or do you mean for variable substitution with empty numeric expression? The paragraph for such case is above:

The syntax to define a numeric variable is `[[#%<fmtspec>,<NUMVAR>:]]` where:

  • `%<fmtspec> is an optional scanf-style matching format specifier to indicate what number format to match (e.g. hex number). Currently accepted format specifiers are %u, %d, %x and %X. If absent, the format specifier defaults to %u`.
  • `<NUMVAR>` is the name of the numeric variable to define to the matching value.

For example:

.. code-block:: llvm

; CHECK: mov r[[#REG:]], 0x[[#%X,IMM:]]

would match `mov r5, 0xF0F0 and set REG to the value 5 and IMM`
to the value `0xF0F0`.

llvm/lib/Support/FileCheck.cpp
714

How about HasParsedValidConstraint? If nothing is parsed the variable remains false which does not contradict the fact that the default constraint applies and is valid. As to the parseNumericOperand function it is sometimes called in a context where the text being parsed cannot be an invalid constraint (e.g. when called from parseBinop for the second operand) and thus we only want to warn about invalid operand, yet no constraint has been parsed. This is why I used MaybeInvalidConstraint. If true, then we should warn about invalid operand format or constraint, if not we should only warn about invalid operand format.

llvm/lib/Support/FileCheckImpl.h
731–736

See above why I did not rename the parameter.

jhenderson accepted this revision.Jun 10 2020, 12:24 AM

LGTM, thanks for all this work! Let me know once Patch 12 (or whatever you replace it with) is ready for review and I'll start looking at it.

llvm/docs/CommandGuide/FileCheck.rst
687–688

So on further review of the doc as a whole, I think there's some redundancy involved. This is actually all one single syntax, yet is split into "numeric definitions" and "numeric substitutions", and then further "definitions with constraints" which is actually just the unification of the two.

Not for this patch, but I think we can reduce the documentation by starting with the unified syntax ([#%<fmtspec>,<NUMVAR>:<constraint>]) and breaking down each part individually:

  • # is required to mark this as a numeric expression as opposed to a string variable use/definition.
  • %<fmtspec> is optional and indicates the format of the variable to be matched. If not specified, the format is derived from the constrain (if present), or inferred from the matched string (or whateer the rule actually is).
  • , is there iff both %<fmtspec> and either <NUMVAR>: or <constraint> are.
  • <NUMVAR>: specifies variable name being defined. Optional, so if not present, no variable is defined.
  • <constraint> if specified limits what this substitution can match to. If not present, anything matched by the %<fmtspec> is permitted.

Everything else just falls out naturally - a definition without any other parts is a definition without constraint. A constraint without a definition just limits what the matched number can be etc. It doesn't hurt to give examples of each of these sub-cases, but I'd start with the combined case and break it down.

llvm/lib/Support/FileCheck.cpp
714

Seems reasonable.

This revision is now accepted and ready to land.Jun 10 2020, 12:24 AM
thopre marked 3 inline comments as done.Jun 10 2020, 5:43 AM
thopre added inline comments.
llvm/docs/CommandGuide/FileCheck.rst
687–688

Funny you should mention that because the documentation used to be like that (albeit without many cleanup that have happen since) and was changed in this split up version. See https://reviews.llvm.org/D49084#1338039.

The directives are also structured like this somewhat with first directives not mentioning variables and or regex. Things get introduced progressively to ease the reading. One thing that could be done is give the full syntax and then break it up into definition and expression, ending with a mixed of the two. That seems to be what you suggest.

jhenderson added inline comments.Jun 10 2020, 6:00 AM
llvm/docs/CommandGuide/FileCheck.rst
687–688

Yes, I think it's a good idea to start with the combined syntax, then split it up, rather than the other way around. That way, it's clear from the start that everything is one single syntax, just with different ways of using it.

This revision was automatically updated to reflect the committed changes.