[PATCH] D60391: FileCheck [11/12]: Add matching constraint specification

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:692-693
+  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.
+
----------------
thopre wrote:
> jhenderson wrote:
> > Perhaps worth clarifying that an empty constraint means any value is accepted.
> 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``.
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.


================
Comment at: llvm/lib/Support/FileCheck.cpp:602
+  Expr = Expr.ltrim(SpaceChars);
+  bool MaybeInvalidConstraint = true;
+  if (Expr.consume_front("=="))
----------------
thopre wrote:
> jhenderson wrote:
> > 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.
> 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.
Seems reasonable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60391/new/

https://reviews.llvm.org/D60391