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

339

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

611–612

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

llvm/lib/Support/FileCheck.cpp

577

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.

588

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

171

Nit: missing full stop.

179

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?

182

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

611–612

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

577

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

611–612

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

577

Seems reasonable.

[thopre]

llvm/docs/CommandGuide/FileCheck.rst

611–612

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

611–612

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 of a numeric substitution is ``[[#%<fmtspec>:<expr>]]`` where:
+The syntax of a numeric substitution is
+``[[#%<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.
 
 Spaces are accepted before, after and between the operands.
 
 For example:

llvm/lib/Support/FileCheck.cpp

FileCheckNumExpr *FileCheckPattern::parseNumericSubstitution(
   DefinedNumericVariable = nullptr;
   // 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.
+  Expr = Expr.ltrim(SpaceChars);
+  StringRef ConstraintExpr = Expr;
+  if (stripFront(Expr, "==", true /*Optional*/))
+    return nullptr;
+
   // Parse numeric expression itself.
   Expr = Expr.ltrim(SpaceChars);
   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 = parseBinop(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

 ; Numeric expressions using variables defined on the command-line and an
 ; 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
 13
 CHECK-LABEL: USE CONV FMT IMPL MATCH

 VAR USE IMPL FMT CONFLICT
 23
 FMT-CONFLICT-LABEL: VAR USE IMPL FMT CONFLICT
 FMT-CONFLICT-NEXT: [[#VAR1 + VAR2]]
 FMT-CONFLICT-MSG: numeric-expression.txt:[[#@LINE-1]]:23: error: variables with conflicting format specifier: need an explicit one
 FMT-CONFLICT-MSG-NEXT: {{F}}MT-CONFLICT-NEXT: {{\[\[#VAR1 \+ VAR2\]\]}}
 FMT-CONFLICT-MSG-NEXT: {{^                      \^$}}
 
-
 ; Numeric expression with overflow
 RUN: not FileCheck -check-prefix OVERFLOW -input-file %s %s 2>&1 \
 RUN:   | FileCheck -check-prefix OVERFLOW-MSG %s
 
 OVERFLOW
 BIGVAR=10000000000000000
 OVERFLOW-LABEL: OVERFLOW
 OVERFLOW-NEXT: BIGVAR: [[#BIGVAR:0x8000000000000000+0x8000000000000000]]
 OVERFLOW-MSG: numeric-expression.txt:[[#@LINE-1]]:27: error: Unable to substitute variable or numeric expression
 OVERFLOW-MSG-NEXT: {{O}}VERFLOW-NEXT: BIGVAR: {{\[\[#BIGVAR:0x8000000000000000\+0x8000000000000000\]\]}}
 OVERFLOW-MSG-NEXT: {{^                          \^$}}
 
-
 ; Numeric expression with underflow
 RUN: not FileCheck -check-prefix UNDERFLOW -input-file %s %s 2>&1 \
 RUN:   | FileCheck -check-prefix UNDERFLOW-MSG %s
 
 UNDERFLOW
 TINYVAR=-10000000000000000
 UNDERFLOW-LABEL: UNDERFLOW
 UNDERFLOW-NEXT: TINYVAR: [[#%d,TINYVAR:-0x8000000000000000-0x8000000000000000]]
 UNDERFLOW-MSG: numeric-expression.txt:[[#@LINE-1]]:29: 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
+RUN: not FileCheck -check-prefix EMPTY-NUMEXPR-CONSTRAINT -input-file %s %s 2>&1 \
+RUN:   | FileCheck -check-prefix EMPTY-NUMEXPR-CONSTRAINT-MSG %s
+
+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]]:38: 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
+RUN: not FileCheck -check-prefix EMPTY-NUMDEF-CONSTRAINT -input-file %s %s 2>&1 \
+RUN:   | FileCheck -check-prefix EMPTY-NUMDEF-CONSTRAINT-MSG %s
+
+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]]:44: 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: {{^                                           \^$}}