FileCheck [11/12]: Add matching constraint specification

[thopre]

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)

[thopre]

Use camel casing for new functions

[thopre]

Rebase

[thopre]

Make numeric value constructor explicit

[thopre]

Rebase on latest changes

[thopre]

Rebase on latest changes

[thopre]

Rebase on latest changes in unittests

[thopre]

Rebase on top of latest changes

[thopre]

Rebase on top of latest changes.

[thopre]

Rebase on top of latest changes

[thopre]

Rebase on top of latest changes

[thopre]

Rebase on top of latest changes

[thopre]

Rebase on top of latest changes

[thopre]

Rebase on top of latest changes

[thopre]

rebase on top of latest changes

[thopre]

rebase on top of latest changes

[thopre]

Rebase on top of latest changes in patch series

[thopre]

Rebase on top of latest changes in the series

[thopre]

Rebase on top of latest changes in patch series

[thopre]

Rebase on latest changes in patch series

[thopre]

Rebase

[thopre]

Need to add unit tests

[thopre]

Rebase

[thopre]

Rebase

[thopre]

Rebase

[thopre]

Need to add unit tests

[jhenderson]

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

llvm/test/FileCheck/numeric-expression.txt

336

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]]

[thopre]

In D60391#2062284, @jhenderson wrote:

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]

• Rebase
• Add unit tests
• Fix logic to detect invalid constraint

[thopre]

Address review comments

[thopre]

Move negative testing just after positive testing

[jhenderson]

llvm/docs/CommandGuide/FileCheck.rst

607–608

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

llvm/lib/Support/FileCheck.cpp

654

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.

665

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

716 ↗

(On Diff #269257)

\ -> /

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

195

Nit: missing full stop.

203

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?

206

Doesn't this need a ProtectFileCheckOutput line?

llvm/unittests/Support/FileCheckTest.cpp

1029 ↗

(On Diff #269257)

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]

Address review comments

llvm/docs/CommandGuide/FileCheck.rst

607–608

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

654

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

716 ↗

(On Diff #269257)

See above why I did not rename the parameter.

[jhenderson]

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

607–608

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

654

Seems reasonable.

[thopre]

llvm/docs/CommandGuide/FileCheck.rst

607–608

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]

llvm/docs/CommandGuide/FileCheck.rst

607–608

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.

llvm/docs/CommandGuide/FileCheck.rst

 .. 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 ``61680``.
 
 The syntax to check a numeric expression constraint is
-``[[#%<fmtspec>:<expr>]]`` where:
+``[[#%<fmtspec>: <constraint> <expr>]]`` where:
 
 * ``%<fmtspec>`` is the same matching format specifier as for defining numeric
   variables but acting as a printf-style format to indicate how a numeric
   expression value should be matched against.  If absent, the format specifier
   is inferred from the matching format of the numeric variable(s) used by the
   expression constraint if any and they are all the same, and default to ``%u``
   if no numeric variable is used.  In case of conflict between matching formats
   of several numeric variables the format specifier is mandatory.
 
+* ``<constraint>`` is the matching constraint describing how the value to match
+  must relate to the value of the numeric expression. The only currently
+  accepted constraint is ``==`` for an exact match and is the default if
+  ``<constraint>`` is not provided. No matching constraint must be specified
+  when the ``<expr>`` is empty.
+
 * ``<expr>>`` is a numeric expression whose operands are either numeric
   variables previously defined or integer immediates.  Currently supported
   numeric operations are ``+`` and ``-``. A single numeric variable or
   integer literal is also accepted.
 
 For example:
 
 .. code-block:: llvm

llvm/lib/Support/FileCheck.cpp

std::shared_ptr<FileCheckNumExpr> FileCheckPattern::parseNumericExpression(
   NumVarDef.reset();
   // Save variable definition expression if any.
   size_t DefEnd = Expr.find(':');
   if (DefEnd != StringRef::npos) {
     DefExpr = Expr.substr(0, DefEnd);
     Expr = Expr.substr(DefEnd + 1);
   }
 
+  // Parse matching constraint.
+  skipWhitespace(Expr);
+  StringRef ConstraintExpr = Expr;
+  if (skip(Expr, "==", true /*Optional*/))
+    return nullptr;
+
   // Parse numeric expression itself.
   skipWhitespace(Expr);
   StringRef UseExpr = Expr;
-  if (!Expr.empty()) {
+  if (Expr.empty()) {
+    if (Expr.size() != ConstraintExpr.size()) {
+      SM.PrintMessage(
+          SMLoc::getFromPointer(Expr.data()), SourceMgr::DK_Error,
+          "Empty numeric expression should not have a matching constraint");
+      return nullptr;
+    }
+  } else {
     // First operand in legacy numeric expression is the @LINE pseudo variable.
     enum AllowedOperand AO = Legacy ? LegacyVar : Any;
     NumExprAST = parseNumericOperand(Expr, AO, SM);
     while (NumExprAST != nullptr && !Expr.empty()) {
       NumExprAST = parseFileCheckBinop(Expr, NumExprAST, Legacy, SM);
       // Legacy numeric expressions only allow 2 operands.
       if (Legacy && !Expr.empty()) {
         SM.PrintMessage(SMLoc::getFromPointer(Expr.data()), SourceMgr::DK_Error,

llvm/test/FileCheck/numeric-expression.txt

 ; RUN: FileCheck -input-file %s %s
 ; RUN: not FileCheck -check-prefixes ERR,INVALID-FMT-SPEC1 -input-file %s %s 2>&1 | FileCheck -check-prefix INVALID-FMT-SPEC-MSG1 %s
 ; RUN: not FileCheck -check-prefixes ERR,INVALID-FMT-SPEC2 -input-file %s %s 2>&1 | FileCheck -check-prefix INVALID-FMT-SPEC-MSG2 %s
 ; RUN: not FileCheck -check-prefix UNDEF-USE -input-file %s %s 2>&1 | FileCheck --strict-whitespace -check-prefix UNDEF-USE-MSG %s
 ; RUN: not FileCheck -check-prefixes CHECK,INVAL-OP -input-file %s %s 2>&1 | FileCheck --strict-whitespace -check-prefix INVAL-OP-MSG %s
 ; RUN: not FileCheck -check-prefixes CONFLICT,CONFLICT1,CONFLICT2 -input-file %s %s 2>&1 | FileCheck --strict-whitespace -check-prefix INPUT-PAT-CONFLICT %s
 ; RUN: not FileCheck -D#NUMVAR=42 -check-prefixes CONFLICT,CONFLICT2 -input-file %s %s 2>&1 | FileCheck --strict-whitespace -check-prefix INPUT-PAT-CONFLICT %s
 ; RUN: not FileCheck -D#NUMVAR=42 -DNUMVAR=foobar -check-prefix CONFLICT -input-file %s %s 2>&1 | FileCheck --strict-whitespace -check-prefix CLI-PAT-CONFLICT %s
 ; RUN: not FileCheck -check-prefixes CONFLICT,CONFLICT3,CONFLICT4 -input-file %s %s 2>&1 | FileCheck --strict-whitespace -check-prefix INPUT-NUM-CONFLICT %s
 ; RUN: not FileCheck -DPATVAR=foobar -check-prefixes CONFLICT,CONFLICT4 -input-file %s %s 2>&1 | FileCheck --strict-whitespace -check-prefix INPUT-NUM-CONFLICT %s
 ; RUN: not FileCheck -DPATVAR=foobar -D#PATVAR=42 -check-prefix CONFLICT -input-file %s %s 2>&1 | FileCheck --strict-whitespace -check-prefix CLI-NUM-CONFLICT %s
 ; RUN: not FileCheck -check-prefix DEF-EXPR-FAIL -input-file %s %s
 ; RUN: not FileCheck -check-prefixes CHECK,FMT-CONFLICT -input-file %s %s 2>&1 | FileCheck --strict-whitespace -check-prefix FMT-CONFLICT-MSG %s
 ; RUN: not FileCheck -check-prefix OVERFLOW -input-file %s %s 2>&1 | FileCheck -check-prefix OVERFLOW-MSG %s
 ; RUN: not FileCheck -check-prefix UNDERFLOW -input-file %s %s 2>&1 | FileCheck -check-prefix UNDERFLOW-MSG %s
+; RUN: not FileCheck -check-prefix EMPTY-NUMEXPR-CONSTRAINT -input-file %s %s 2>&1 | FileCheck -check-prefix EMPTY-NUMEXPR-CONSTRAINT-MSG %s
+; RUN: not FileCheck -check-prefix EMPTY-NUMDEF-CONSTRAINT -input-file %s %s 2>&1 | FileCheck -check-prefix EMPTY-NUMDEF-CONSTRAINT-MSG %s
 
 
 ; We ensure we attemt to match all lines with digits by using CHECK-NEXT
 ; directives for the checks to ensure each line of input is covered.
 
 
 ; Numeric variable definition with default matching format
 DEF DEFAULT FMT

 ; immediate interpreted as an unsigned value
 ; Note: 9223372036854775819 = 0x8000000000000000 + 11
 USE IMPL FMT IMPL MATCH UNSIGNED IMM
 9223372036854775819
 ; CHECK-LABEL: USE IMPL FMT IMPL MATCH UNSIGNED IMM
 ; CHECK-NEXT: [[#VAR1+0x8000000000000000]]
 
 
+; Numeric expressions in default matching format and explicit matching rule using
+; variables defined on other lines
+USE DEF FMT EXPL MATCH
+11
+11
+11
+; CHECK-LABEL: USE DEF FMT EXPL MATCH
+; CHECK-NEXT: [[#==VAR1]]
+; CHECK-NEXT: [[# ==VAR1]]
+; CHECK-NEXT: [[# == VAR1]]
+
+
 ; Numeric expressions with conversion matching format and implicit matching rule
 ; using variables defined on other lines
 USE CONV FMT IMPL MATCH
 b
 B
 12
 12
 13

 ; OVERFLOW-MSG-NEXT: {{^                            \^$}}
 
 
 ; Numeric expression with underflow
 UNDERFLOW
 TINYVAR=-10000000000000000
 ; UNDERFLOW-LABEL: UNDERFLOW
 ; UNDERFLOW-NEXT: TINYVAR: [[#%d,TINYVAR:-0x8000000000000000-0x8000000000000000]]
 ; UNDERFLOW-MSG: numeric-expression.txt:[[#@LINE-1]]:31: error: Unable to substitute variable or numeric expression
 ; UNDERFLOW-MSG-NEXT: {{U}}NDERFLOW-NEXT: TINYVAR: {{\[\[#%d,TINYVAR:-0x8000000000000000-0x8000000000000000\]\]}}
 ; UNDERFLOW-MSG-NEXT: {{^                              \^$}}
+
+
+; Empty numeric expression with matching constraint
+EMPTY NUMEXPR USE WITH CONSTRAINT
+18
+; EMPTY-NUMEXPR-CONSTRAINT-LABEL: EMPTY NUMEXPR USE WITH CONSTRAINT
+; EMPTY-NUMEXPR-CONSTRAINT-NEXT: [[# ==]]
+; EMPTY-NUMEXPR-CONSTRAINT-MSG: numeric-expression.txt:[[#@LINE-1]]:40: error: Empty numeric expression should not have a matching constraint
+; EMPTY-NUMEXPR-CONSTRAINT-MSG-NEXT: {{E}}MPTY-NUMEXPR-CONSTRAINT-NEXT: {{\[\[# ==\]\]}}
+; EMPTY-NUMEXPR-CONSTRAINT-MSG-NEXT: {{^                                       \^$}}
+
+
+; Empty numeric expression with matching constraint
+EMPTY NUMEXPR DEF WITH CONSTRAINT
+18
+; EMPTY-NUMDEF-CONSTRAINT-LABEL: EMPTY NUMEXPR CONSTRAINT
+; EMPTY-NUMDEF-CONSTRAINT-NEXT: [[#VARDEF: ==]]
+; EMPTY-NUMDEF-CONSTRAINT-MSG: numeric-expression.txt:[[#@LINE-1]]:46: error: Empty numeric expression should not have a matching constraint
+; EMPTY-NUMDEF-CONSTRAINT-MSG-NEXT: {{E}}MPTY-NUMDEF-CONSTRAINT-NEXT: {{\[\[#VARDEF: ==\]\]}}
+; EMPTY-NUMDEF-CONSTRAINT-MSG-NEXT: {{^                                             \^$}}