FileCheck [7/12]: Arbitrary long numeric expressions

[thopre]

Summary

This patch is part of a patch series to add support for FileCheck
numeric expressions. This specific patch extend numeric expression to
support an arbitrary number of operands, either variable or literals.

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]

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]

Get rid of the useless FileCheckNumExpr class which is only needed when matching format are introduced

[thopre]

Rebase on top of latest changes in patch series

[thopre]

Rebase on top of latest changes in the series

[arichardson]

llvm/include/llvm/Support/FileCheck.h

105 ↗

(On Diff #203209)

Maybe this should use Expected instead of Optional to report errors?

llvm/lib/Support/FileCheck.cpp

738 ↗

(On Diff #203209)

Could this be replaced with Error/Expected so that there is no more need for getUndefVarNames/appendUndefVarNames calls?

[jhenderson]

I've reviewed up to the tests, and got lots of comments for you.

llvm/docs/CommandGuide/FileCheck.rst

594–595 ↗

(On Diff #203209)

numeric variables previously defined -> previously defined numeric variables

llvm/include/llvm/Support/FileCheck.h

43 ↗

(On Diff #203209)

Are there other kinds of expressions except "numeric expressions"? Just wondering if we can simplify the terminology a little e.g. by simply referring to them as expressions and the FileCheckNumExprAST can become FileCheckExpressionAST.

66–67 ↗

(On Diff #203209)

Are there plans for signed literals later? If not, then I'd get rid of "unsigned" from the comment.

69–70 ↗

(On Diff #203209)

You could probably simplify this down to \returns the literal's value.

103–104 ↗

(On Diff #203209)

Similar comment to above. No need to explain that its evaluating the expression for a single variable. Just keep the "returns this variable's value" part.

130 ↗

(On Diff #203209)

Do these need to be shared_ptrs? Do multiple places need ownership, or is it sufficient that only place has it? Same question applies for all uses of shared_ptr further down.

152 ↗

(On Diff #203209)

Could this use a MutableArrayRef?

193–194 ↗

(On Diff #203209)

How about "the name of the undefined variables used in this substitution"?

Also use "Appends" instead of "Records" to show that it doesn't override the old ones.

Same comments apply to the other examples of getUndefVarNames.

209–210 ↗

(On Diff #203209)

What if some string variables are defined and others aren't? The comment as it stands suggests that all are stored. If that's not intended, I'd rephrase it as "the name of the undefined string variables used in this substitution"

441 ↗

(On Diff #203209)

What's a "legacy numeric substitution block"?

446 ↗

(On Diff #203209)

defined -> define

509 ↗

(On Diff #203209)

I don't like the terminology "Legacy", as it doesn't convey any useful meaning, unless defined somewhere clearly.

516 ↗

(On Diff #203209)

I don't think you need enum here. Same applies throughout the code.

llvm/lib/Support/FileCheck.cpp

81 ↗

(On Diff #203209)

Are you sure you want to clear this? That seems unnecessary, and confusing if you are passing in a reference. If you want an empty vector, I'd just expect the vector to be returned.

208 ↗

(On Diff #203209)

Get rid of this blank line.

222 ↗

(On Diff #203209)

Magic number?

[thopre]

• Rebase on latest changes in patch series
• Address obvious changes

[thopre]

llvm/include/llvm/Support/FileCheck.h

43 ↗

(On Diff #203209)

There's only numeric expression indeed but wouldn't the use of "expression" only induce the reader into thinking it only applies when there is an operator involved? Numeric expression as it stands also cover a single numeric variable in a numeric substitution. Also if changing to expression only, should I remove the "numeric" adjective in the documentation as well?

66–67 ↗

(On Diff #203209)

There are but the comment can be changed when they are introduced. Fixed.

130 ↗

(On Diff #203209)

Some AST nodes are referenced from a single place only (eg. literals and binary operations) while numeric variables can be shared among several numeric substitutions, eg. "add r[[#REG]], r[[#REG+1]], 42" would have 2 pointers to the class instance for REG. In the first case std::unique_ptr could be used, while in the latter case std::shared_ptr would make sense.

The problem comes from AST nodes being referred via FileCheckAST * pointers (since each operand can be either of the possible nodes) and thus I need to decide on a given pointer type for all nodes. I could go with regular pointers and keep an array of pointer in FileCheckPatternContext to all nodes created for them to be freed all at once when matching is done. Wouldn't change much over what is happening now since substitutions are only deleted once all pattern have been matched and substitutions points to the AST indirectly. Does that sound good?

441 ↗

(On Diff #203209)

It refers to the "legacy use of @LINE" mentioned in the section "FileCheck Pseudo Numeric Variables" of the documentation, namely [[@LINE]] and [[@LINE+offset]] instead of the same with a '#' sign. Would "indicates whether Expr is a legacy use of @LINE" be clearer? Or perhaps "legacy @LINE expression"? If not, any other suggestion?

509 ↗

(On Diff #203209)

See above.

[probinson]

Regarding what to call the @LINE+offset form, I think it's fine to call it "legacy" in the user-facing documentation, but it gets to be a bit much in the internals. I haven't commented every use but you will get the idea in the inline comments.

llvm/docs/CommandGuide/FileCheck.rst

593 ↗

(On Diff #204497)

<expr>> should be <expr>

597 ↗

(On Diff #204497)

There are a few different ways to describe expressions, I happen to prefer a recursive definition, something like this:

A numeric expression is recursively defined as:

• a numeric operand, or
• a numeric expression followed by an operator and a numeric operand.

A numeric operand is a previously defined numeric variable, or an integer literal. The supported operators are + and -. Spaces are accepted before, after and between any of these elements.

llvm/include/llvm/Support/FileCheck.h

538 ↗

(On Diff #204497)

These could be { LineVar, Literal, Any } perhaps.

540 ↗

(On Diff #204497)

I don't see a parameter named AllowedOperandFlag.

548 ↗

(On Diff #204497)

\p LineExpr ... parsing a @LINE expression.

43 ↗

(On Diff #203209)

I think "expression" does not necessarily imply an operator, at least not to a programmer.
You can probably also remove some uses of "numeric" in the documentation, when you are talking about expressions (but keep it when talking about variables).

441 ↗

(On Diff #203209)

I don't know about James but I would prefer that the commentary say "legacy @LINE expression" and the parameter could be named LineExpr.

llvm/lib/Support/FileCheck.cpp

189 ↗

(On Diff #204497)

The previous if guarantees AO is LegacyVar or Any here. So, this condition simplifies to AO == LegacyVar correct? (or LineVar if you rename as suggested.)

191 ↗

(On Diff #204497)

Maybe a comment here, "Ignore the error and retry parsing as a literal."

194 ↗

(On Diff #204497)

I think the only possibilities at this point are LegacyLiteral and Any, so this if is always true and should be removed.

244 ↗

(On Diff #204497)

... in a @LINE expression ...

427 ↗

(On Diff #204497)

IsLineExpr

719 ↗

(On Diff #204497)

Delete the commented-out assertion.

727 ↗

(On Diff #204497)

llvm_unreachable instead of assert(false)?

[thopre]

Address all comments but shared_ptr

[thopre]

llvm/include/llvm/Support/FileCheck.h

441 ↗

(On Diff #203209)

I went for LegacyLineExpr then since when false @LINE is also allowed and when true it limits what can be done with @LINE.

llvm/lib/Support/FileCheck.cpp

194 ↗

(On Diff #204497)

I've addressed this comment and the other one about AO above but I wanted to say that although I realize that LLVM code style aims at reducing clutter to make the code more readable, I feel in many case (eg. here and some of the suppression of else after return) it makes the code less explicit and more fragile.

[thopre]

Rebase

[jhenderson]

Apologies for the delay in coming back to this. It's been a busy few days.

llvm/include/llvm/Support/FileCheck.h

67–68 ↗

(On Diff #205484)

I think "Class to represent an undefined variable error, which quotes that variable's name when printed." would be a bit more concise.

94 ↗

(On Diff #205484)

definition -> definitions

121 ↗

(On Diff #205484)

If you made this eval() return an error if the variable is undefined, it would nicely separate concerns of use versus definition (see my comment a bit further down about shared_ptrs).

158 ↗

(On Diff #205484)

This should say "returns an error if a subexpression returns an error, or ..."

480 ↗

(On Diff #205484)

substitued -> substituted

549 ↗

(On Diff #205484)

Perhaps worth making this an enum class.

553 ↗

(On Diff #205484)

"against \p SM"? I'm not sure I understand this change in terminology.

130 ↗

(On Diff #203209)

I wonder whether it might make more sense to separate the concept of variables from the AST. The AST could have a variable reference class in it, which refers by raw pointer to a variable definition. An AST expression then solely "owns" each subexpression, whilst something higher up (e.g. the Context - not sure about this), owns the variables and contains the definitions. That way each item in the tree is only owned by its parents, which feels more logical.

Does that make sense?

As things stand the fact that a single FileCheckNumericVariable can be owned by multiple places implies it's not really a tree at all. (In graph theory a tree has no cross-references - everything is owned by a single parent only).

441 ↗

(On Diff #203209)

How about IsLegacyLineExpr? It's a bit more verbose but maybe a little clearer too. Otherwise LegacyLineExpr is okay. It's just that the variable isn't actually an expression itself.

llvm/lib/Support/FileCheck.cpp

198 ↗

(On Diff #205484)

I'm guessing there is a plan to allow other Radixes too? It seems like it would be normal to want to use hex, for example.

243 ↗

(On Diff #205484)

in a legacy

270 ↗

(On Diff #205484)

Err would be a more common variable name.

[jhenderson]

llvm/lib/Support/FileCheck.cpp

290 ↗

(On Diff #205484)

in a legacy

715 ↗

(On Diff #205484)

Trailing full stop required.

725–727 ↗

(On Diff #205484)

I think this is actually unnecessary. handleAllErrors terminates the program with an llvm_unreachable anyway if there are any unhandled errors.

[thopre]

Make new function parameter immediate use the /*ParamName=*/Value code style

[probinson]

I think I am happy with this, leaving the rest up to James.

llvm/include/llvm/Support/FileCheck.h

158 ↗

(On Diff #205711)

I prefer the description of the return value to give the expected/normal case first, and the error case afterward. So,

\returns the expression value, or an error ...

[thopre]

• Rebase on top of refactoring patches
• Address comments

llvm/include/llvm/Support/FileCheck.h

553 ↗

(On Diff #205484)

I've used it in other place already. I'm using "against" because the diagnostic relates to the buffer held by SM (it is created via SM.GetMessage)

llvm/lib/Support/FileCheck.cpp

198 ↗

(On Diff #205484)

/There is yes. See patch 9/12

[thopre]

Rebase on latest changes in patch series

[thopre]

Rebase on top of latest changes in patch series

[jhenderson]

Mostly looks good. A few nits still and one or two more significant comments. I think this structure looks much cleaner than the previous version, personally.

llvm/include/llvm/Support/FileCheck.h

54 ↗

(On Diff #208184)

As things stand, this only can represent an unsigned literal. I think you'll want to support negative literals or unary negation operands fairly quickly. Otherwise, the expression "-1" doesn't work.

I'm also slightly concerned in the current form that somebody could try to use FileCheckExpressionLiteral for negative numbers, and get surprising results. Perhaps worth updating the comment to say that the class represents an unsigned integer literal.

60 ↗

(On Diff #208184)

Perhaps "Construct a literal with the specified value."

llvm/lib/Support/FileCheck.cpp

49 ↗

(On Diff #208184)

Might be worth making this comment more generic e.g. "Handle a failed operation, e.g. due to an undefined variable." That'll make the comment robust to other possible future failure types.

258 ↗

(On Diff #208184)

is always a literal

The comment should probably start with "The second..."

301 ↗

(On Diff #208184)

The comment should probably start with "The first..."

722 ↗

(On Diff #208184)

You've changed this message implying it can have more than one undefined variable, but I don't see any lit test case change that has more than one. You should add/update one.

llvm/unittests/Support/FileCheckTest.cpp

17 ↗

(On Diff #208184)

I think it would be a good idea to have a test case showing that this can take std::numeric_limits<uint64_t>::max().

25 ↗

(On Diff #208184)

const std::set &?

30 ↗

(On Diff #208184)

Perhaps worth having this print what names are still remaining if not empty. I think you can do something like EXPECT_TRUE(ExpectedUndefVarNames.empty()) << ExpectedUndefVarNames.

50 ↗

(On Diff #208184)

getValue not getvalue.

397 ↗

(On Diff #208184)

In other places you've used bool(SubstValue) for this sort of conversion. I think that might be nicer, but don't care that much. Regardless, it should be consistent throughout.

479 ↗

(On Diff #208184)

unique_ptr?

[thopre]

Address review comments.

[thopre]

llvm/include/llvm/Support/FileCheck.h

553 ↗

(On Diff #205484)

Is my explanation satisfying or do you want to use another phrasing?

llvm/lib/Support/FileCheck.cpp

722 ↗

(On Diff #208184)

There was a unit test already but it doesn't test that message indeed. I've added 1 lit test and updated another one, the former for correct substitution and the latter for undefined variables.

llvm/unittests/Support/FileCheckTest.cpp

25 ↗

(On Diff #208184)

This cannot be const since erase is called on that set. Making it a reference means the set needs to be created in all the caller when now the caller can provide an initializer as is done in expectUndefError below.

[thopre]

Run clang-format-diff

[jhenderson]

llvm/include/llvm/Support/FileCheck.h

553 ↗

(On Diff #205484)

Yes, it's fine. Not the phrasing I would use but I don't care much about it.

60 ↗

(On Diff #208978)

litteral -> literal

llvm/lib/Support/FileCheck.cpp

49 ↗

(On Diff #208978)

eg. -> e.g.
either "an undefined variable" or "undefined variables"

llvm/unittests/Support/FileCheckTest.cpp

19 ↗

(On Diff #208978)

Why not FileCheckExpressionLiteral Ten(10)?

25 ↗

(On Diff #208978)

Why not FileCheckExpressionLiteral Max(...)?

33 ↗

(On Diff #208978)

Other instances of overloading operator<< in the LLVM unit tests tend to just be in the llvm namespace, not in std. See for example ADT/StringRefTest.cpp.

The variables in this function don't use LLVM naming style.

35 ↗

(On Diff #208978)

StringRef?

47 ↗

(On Diff #208978)

Use std::unordered_set?

[grimar]

llvm/lib/Support/FileCheck.cpp

427 ↗

(On Diff #204497)

I'd suggest adding a comment.

47 ↗

(On Diff #208978)

Doesn't look like you need this->.

199 ↗

(On Diff #208978)

btw it was a bit confusing for me to find there is a method that take bool IsPseudo
and method that take bool &IsPseudo, i.e. that one of them can change this flag, but another - not.

201 ↗

(On Diff #208978)

You can avoid having this temorarily variable:

if (ParseVarResult)
  return parseNumericVariableUse(*ParseVarResult, IsPseudo, SM);

But actually it seems you also need to check the result of Expected,
not sure why it is assumed there is no error?

llvm/unittests/Support/FileCheckTest.cpp

34 ↗

(On Diff #208978)

Seems a different variables naming style used here? Ten, Value vs first, set. You need stick to one I think.
(To upper case I guess? LLVM did not do a switch to a new one yet, except LLD project I believe).

[grimar]

llvm/lib/Support/FileCheck.cpp

201 ↗

(On Diff #208978)

Oh, sorry, I misread the code, you actually check it. Then you still can avoid having the temp var :)

[thopre]

llvm/lib/Support/FileCheck.cpp

199 ↗

(On Diff #208978)

You mean parseVariable Vs parseNumericVariableUse? Would you rather me have both be reference even though the latter only needs to get the value? Or is there a naming convention to denote a reference as opposed to a parameter passed by value?

llvm/unittests/Support/FileCheckTest.cpp

33 ↗

(On Diff #208978)

I'm not sure why but when I try with llvm namespace and I switch to unordered_set as suggested below the overloaded operator does not get used and the code fails to compile. However the code compiles and works with llvm namespace if I use a set instead of unordered_set.

I've thus decided to just write a setToString method and output the result of calling it. It does create lots of temporaries though.

[thopre]

Address review comments

[jhenderson]

llvm/unittests/Support/FileCheckTest.cpp

32 ↗

(On Diff #209237)

static?

Though saying that, this is inside an anonymous namespace, so perhaps the other functions need static removing.

This function returns a StringRef, yet the base type is a local variable. I think that will result in the memory being freed under the reference. Perhaps better would be to make Str a Twine and then use Twine::str() to return a std::string.

38 ↗

(On Diff #209237)

Perhaps make this ", ", and start and end the full string with '{'/'}' to make it easier to read.

[grimar]

llvm/include/llvm/Support/FileCheck.h

580 ↗

(On Diff #209237)

This comment still mentions Expr and SM.

llvm/lib/Support/FileCheck.cpp

199 ↗

(On Diff #208978)

There is no special naming conventions, but given that parseVariable method always must set IsPseudo flag,
but another parse* methods can only read the flags they are given, I wonder if it be better/cleaner for parseVariable to return Expected<std::pair<StringRef, bool> for example?

[thopre]

Address review comments

llvm/lib/Support/FileCheck.cpp

199 ↗

(On Diff #208978)

I'm not a big fan of pairs because its members are accessed without a name (first or second) which I find it less readable. Anyway, done but I've reverted the change to make the call to parseNumericVariableUse a one liner so that it is clear what is being passed to that function (ParseVarResult->first is not very descriptive).

llvm/unittests/Support/FileCheckTest.cpp

32 ↗

(On Diff #209237)

Ah yes indeed, the StringRef does not own the string it references. I cannot use a Twine for Str because you cannot store a Twine so while I can do Str + S, I cannot store the result in Str (see https://llvm.org/doxygen/classllvm_1_1Twine.html#a28b850ffd65d5997cf730d644e06d89b I've been bitten by this before in this patchset). I guess I'll return a std::string then. I've used a Twine to do the " " concatenation though.

[jhenderson]

llvm/lib/Support/FileCheck.cpp

199 ↗

(On Diff #208978)

You could use a small struct to name the members, perhaps called VariableProperties:

struct VariableProperties {
  StringRef Name;
  bool IsPseudo;
}

[thopre]

Return a VariableProperties structure in parseVariable

[jhenderson]

LGTM. Thanks for the hard work on this!

llvm/lib/Support/FileCheck.cpp

108 ↗

(On Diff #209481)

You might be able to just do return {Name, IsPseudo} to avoid the temporary.

[thopre]

llvm/lib/Support/FileCheck.cpp

108 ↗

(On Diff #209481)

I tried but it didn't work. I think it would if I was returning the structure, but here I'm returning an Expected.

As per 6.6.3 C++11 draft:
"A return statement with a braced-init-list initializes the object or reference to be returned from the function by copy-list-initialization (8.5.4) from the specified initializer list."

So it tries to build an Expected with that initialization list which fails because I think an Expected<foo> only accepts a Foo.

[thopre]

llvm/lib/Support/FileCheck.cpp

108 ↗

(On Diff #209481)

Yes, that's it:

/home/thomasp/repos/llvm-project/llvm/lib/Support/FileCheck.cpp:107:10: error: no matching constructor for initialization of 'Expected<FileCheckPattern::VariableProperties>'

return {Name, IsPseudo};
       ^~~~~~~~~~~~~~~~

/home/thomasp/repos/llvm-project/llvm/include/llvm/Support/Error.h:474:3: note: candidate template ignored: requirement 'std::is_convertible<StringRef &, VariableProperties>::value' was not satisfied [with OtherT = llvm::StringRef &]

Expected(OtherT &&Val,
^

It tries to convert Name into a VariableProperties and fails. All the other constructors expect an Error or another Expected and fail as well.

[arichardson]

Thanks for your work on this! Looking forward to be able to use these new features the next time I update our fork.

llvm/lib/Support/FileCheck.cpp

108 ↗

(On Diff #209481)

Maybe return VariableProperties{Name, IsPseudo}would work?

[jhenderson]

llvm/lib/Support/FileCheck.cpp

108 ↗

(On Diff #209481)

Yes of course. I think you could still do it inline if you want, (not tried, so not 100% certain off the top of my head) and don't mind either way:

return VariableProperties {Name, IsPseudo};

[thopre]

Return structure in parseVariable as a temporary

llvm/trunk/docs/CommandGuide/FileCheck.rst

.. option:: --enable-var-scope
 
   All other variables get undefined after each encountered ``CHECK-LABEL``.
 
 .. option:: -D<VAR=VALUE>
 
   Sets a filecheck pattern variable ``VAR`` with value ``VALUE`` that can be
   used in ``CHECK:`` lines.
 
-.. option:: -D#<NUMVAR>=<VALUE>
+.. option:: -D#<NUMVAR>=<VALUE EXPRESSION>
 
-  Sets a filecheck numeric variable ``NUMVAR`` to ``<VALUE>`` that can be used
-  in ``CHECK:`` lines.
+  Sets a filecheck numeric variable ``NUMVAR`` to the result of evaluating
+  ``<VALUE EXPRESSION>`` that can be used in ``CHECK:`` lines. See section
+  ``FileCheck Numeric Variables and Expressions`` for details on the format
+  and meaning of ``<VALUE EXPRESSION>``.
 
 .. option:: -version
 
  Show the version number of this program.
 
 .. option:: -v
 
   Print good directive pattern matches.  However, if ``-input-dump=fail`` or

 For example:
 
 .. code-block:: llvm
 
     ; CHECK: mov r[[#REG:]], 42
 
 would match ``mov r5, 42`` and set ``REG`` to the value ``5``.
 
-The syntax of a numeric substitution is ``[[#<NUMVAR><op><offset>]]`` where:
+The syntax of a numeric substitution is ``[[#<expr>]]`` where ``<expr>`` is an
+expression. An expression is recursively defined as:
 
-* ``<NUMVAR>`` is the name of a defined numeric variable.
+* a numeric operand, or
+* an expression followed by an operator and a numeric operand.
 
-* ``<op>`` is an optional operation to perform on the value of ``<NUMVAR>``.
-  Currently supported operations are ``+`` and ``-``.
-
-* ``<offset>`` is the immediate value that constitutes the second operand of
-  the operation ``<op>``. It must be present if ``<op>`` is present, absent
-  otherwise.
-
-Spaces are accepted before, after and between any of these elements.
+A numeric operand is a previously defined numeric variable, or an integer
+literal. The supported operators are ``+`` and ``-``. Spaces are accepted
+before, after and between any of these elements.
 
 For example:
 
 .. code-block:: llvm
 
     ; CHECK: load r[[#REG:]], [r0]
     ; CHECK: load r[[#REG+1]], [r1]
 

llvm/trunk/include/llvm/Support/FileCheck.h

struct FileCheckRequest {
   bool Verbose = false;
   bool VerboseVerbose = false;
 };
 
 //===----------------------------------------------------------------------===//
 // Numeric substitution handling code.
 //===----------------------------------------------------------------------===//
 
+/// Base class representing the AST of a given expression.
+class FileCheckExpressionAST {
+public:
+  virtual ~FileCheckExpressionAST() = default;
+
+  /// Evaluates and \returns the value of the expression represented by this
+  /// AST or an error if evaluation fails.
+  virtual Expected<uint64_t> eval() const = 0;
+};
+
+/// Class representing an unsigned literal in the AST of an expression.
+class FileCheckExpressionLiteral : public FileCheckExpressionAST {
+private:
+  /// Actual value of the literal.
+  uint64_t Value;
+
+public:
+  /// Constructs a literal with the specified value.
+  FileCheckExpressionLiteral(uint64_t Val) : Value(Val) {}
+
+  /// \returns the literal's value.
+  Expected<uint64_t> eval() const { return Value; }
+};
+
+/// Class to represent an undefined variable error, which quotes that
+/// variable's name when printed.
+class FileCheckUndefVarError : public ErrorInfo<FileCheckUndefVarError> {
+private:
+  StringRef VarName;
+
+public:
+  static char ID;
+
+  FileCheckUndefVarError(StringRef VarName) : VarName(VarName) {}
+
+  StringRef getVarName() const { return VarName; }
+
+  std::error_code convertToErrorCode() const override {
+    return inconvertibleErrorCode();
+  }
+
+  /// Print name of variable associated with this error.
+  void log(raw_ostream &OS) const override {
+    OS << "\"";
+    OS.write_escaped(VarName) << "\"";
+  }
+};
+
 /// Class representing a numeric variable and its associated current value.
 class FileCheckNumericVariable {
 private:
   /// Name of the numeric variable.
   StringRef Name;
 
   /// Value of numeric variable, if defined, or None otherwise.
   Optional<uint64_t> Value;
public:
   /// Clears value of this numeric variable, regardless of whether it is
   /// currently defined or not.
   void clearValue();
 
   /// \returns the line number where this variable is defined.
   size_t getDefLineNumber() { return DefLineNumber; }
 };
 
-/// Type of functions evaluating a given binary operation.
-using binop_eval_t = uint64_t (*)(uint64_t, uint64_t);
-
-/// Class to represent an undefined variable error which prints that variable's
-/// name between quotes when printed.
-class FileCheckUndefVarError : public ErrorInfo<FileCheckUndefVarError> {
+/// Class representing the use of a numeric variable in the AST of an
+/// expression.
+class FileCheckNumericVariableUse : public FileCheckExpressionAST {
 private:
-  StringRef VarName;
-
-public:
-  static char ID;
-
-  FileCheckUndefVarError(StringRef VarName) : VarName(VarName) {}
+  /// Name of the numeric variable.
+  StringRef Name;
 
-  StringRef getVarName() const { return VarName; }
+  /// Pointer to the class instance for the variable this use is about.
+  FileCheckNumericVariable *NumericVariable;
 
-  std::error_code convertToErrorCode() const override {
-    return inconvertibleErrorCode();
-  }
+public:
+  FileCheckNumericVariableUse(StringRef Name,
+                              FileCheckNumericVariable *NumericVariable)
+      : Name(Name), NumericVariable(NumericVariable) {}
 
-  /// Print name of variable associated with this error.
-  void log(raw_ostream &OS) const override {
-    OS << "\"";
-    OS.write_escaped(VarName) << "\"";
-  }
+  /// \returns the value of the variable referenced by this instance.
+  Expected<uint64_t> eval() const;
 };
 
-/// Class representing an expression consisting of either a single numeric
-/// variable or a binary operation between a numeric variable and an
-/// immediate.
-class FileCheckExpression {
+/// Type of functions evaluating a given binary operation.
+using binop_eval_t = uint64_t (*)(uint64_t, uint64_t);
+
+/// Class representing a single binary operation in the AST of an expression.
+class FileCheckASTBinop : public FileCheckExpressionAST {
 private:
   /// Left operand.
-  FileCheckNumericVariable *LeftOp;
+  std::unique_ptr<FileCheckExpressionAST> LeftOperand;
 
   /// Right operand.
-  uint64_t RightOp;
+  std::unique_ptr<FileCheckExpressionAST> RightOperand;
 
   /// Pointer to function that can evaluate this binary operation.
   binop_eval_t EvalBinop;
 
 public:
-  FileCheckExpression(binop_eval_t EvalBinop,
-                      FileCheckNumericVariable *OperandLeft,
-                      uint64_t OperandRight)
-      : LeftOp(OperandLeft), RightOp(OperandRight), EvalBinop(EvalBinop) {}
-
-  /// Evaluates the value of this expression, using EvalBinop to perform the
-  /// binary operation it consists of. \returns an error if the numeric
-  /// variable used is undefined, or the expression value otherwise.
+  FileCheckASTBinop(binop_eval_t EvalBinop,
+                    std::unique_ptr<FileCheckExpressionAST> LeftOp,
+                    std::unique_ptr<FileCheckExpressionAST> RightOp)
+      : EvalBinop(EvalBinop) {
+    LeftOperand = std::move(LeftOp);
+    RightOperand = std::move(RightOp);
+  }
+
+  /// Evaluates the value of the binary operation represented by this AST,
+  /// using EvalBinop on the result of recursively evaluating the operands.
+  /// \returns the expression value or an error if an undefined numeric
+  /// variable is used in one of the operands.
   Expected<uint64_t> eval() const;
 };
 
 class FileCheckPatternContext;
 
 /// Class representing a substitution to perform in the RegExStr string.
 class FileCheckSubstitution {
 protected:
public:
   /// when defined, or an error if the variable is undefined.
   Expected<std::string> getResult() const override;
 };
 
 class FileCheckNumericSubstitution : public FileCheckSubstitution {
 private:
   /// Pointer to the class representing the expression whose value is to be
   /// substituted.
-  FileCheckExpression *Expression;
+  std::unique_ptr<FileCheckExpressionAST> ExpressionAST;
 
 public:
-  FileCheckNumericSubstitution(FileCheckPatternContext *Context,
-                               StringRef ExpressionStr,
-                               FileCheckExpression *Expression,
+  FileCheckNumericSubstitution(FileCheckPatternContext *Context, StringRef Expr,
+                               std::unique_ptr<FileCheckExpressionAST> ExprAST,
                                size_t InsertIdx)
-      : FileCheckSubstitution(Context, ExpressionStr, InsertIdx),
-        Expression(Expression) {}
+      : FileCheckSubstitution(Context, Expr, InsertIdx) {
+    ExpressionAST = std::move(ExprAST);
+  }
 
   /// \returns a string containing the result of evaluating the expression in
   /// this substitution, or an error if evaluation failed.
   Expected<std::string> getResult() const override;
 };
 
 //===----------------------------------------------------------------------===//
 // Pattern handling code.
private:
   /// NumericVariableDefs table in the FileCheckPattern instance of that
   /// pattern.
   StringMap<FileCheckNumericVariable *> GlobalNumericVariableTable;
 
   /// Pointer to the class instance representing the @LINE pseudo variable for
   /// easily updating its value.
   FileCheckNumericVariable *LineVariable = nullptr;
 
-  /// Vector holding pointers to all parsed expressions. Used to automatically
-  /// free the expressions once they are guaranteed to no longer be used.
-  std::vector<std::unique_ptr<FileCheckExpression>> Expressions;
-
   /// Vector holding pointers to all parsed numeric variables. Used to
   /// automatically free them once they are guaranteed to no longer be used.
   std::vector<std::unique_ptr<FileCheckNumericVariable>> NumericVariables;
 
   /// Vector holding pointers to all substitutions. Used to automatically free
   /// them once they are guaranteed to no longer be used.
   std::vector<std::unique_ptr<FileCheckSubstitution>> Substitutions;
 
public:
 
   /// Undefines local variables (variables whose name does not start with a '$'
   /// sign), i.e. removes them from GlobalVariableTable and from
   /// GlobalNumericVariableTable and also clears the value of numeric
   /// variables.
   void clearLocalVars();
 
 private:
-  /// Makes a new expression instance and registers it for destruction when
-  /// the context is destroyed.
-  FileCheckExpression *makeExpression(binop_eval_t EvalBinop,
-                                      FileCheckNumericVariable *OperandLeft,
-                                      uint64_t OperandRight);
-
   /// Makes a new numeric variable and registers it for destruction when the
   /// context is destroyed.
   template <class... Types>
   FileCheckNumericVariable *makeNumericVariable(Types... args);
 
   /// Makes a new string substitution and registers it for destruction when the
   /// context is destroyed.
   FileCheckSubstitution *makeStringSubstitution(StringRef VarName,
                                                 size_t InsertIdx);
 
   /// Makes a new numeric substitution and registers it for destruction when
   /// the context is destroyed.
   FileCheckSubstitution *
   makeNumericSubstitution(StringRef ExpressionStr,
-                          FileCheckExpression *Expression, size_t InsertIdx);
+                          std::unique_ptr<FileCheckExpressionAST> ExpressionAST,
+                          size_t InsertIdx);
 };
 
 /// Class to represent an error holding a diagnostic with location information
 /// used when printing it.
 class FileCheckErrorDiagnostic : public ErrorInfo<FileCheckErrorDiagnostic> {
 private:
   SMDiagnostic Diagnostic;
 
public:
   SMLoc getLoc() const { return PatternLoc; }
 
   /// \returns the pointer to the global state for all patterns in this
   /// FileCheck instance.
   FileCheckPatternContext *getContext() const { return Context; }
 
   /// \returns whether \p C is a valid first character for a variable name.
   static bool isValidVarNameStart(char C);
+
+  /// Parsing information about a variable.
+  struct VariableProperties {
+    StringRef Name;
+    bool IsPseudo;
+  };
+
   /// Parses the string at the start of \p Str for a variable name. \returns
-  /// an error holding a diagnostic against \p SM if parsing fail, or the
-  /// name of the variable otherwise. In the latter case, sets \p IsPseudo to
-  /// indicate if it is a pseudo variable and strips \p Str from the variable
-  /// name.
-  static Expected<StringRef> parseVariable(StringRef &Str, bool &IsPseudo,
+  /// a VariableProperties structure holding the variable name and whether it
+  /// is the name of a pseudo variable, or an error holding a diagnostic
+  /// against \p SM if parsing fail. If parsing was successful, also strips
+  /// \p Str from the variable name.
+  static Expected<VariableProperties> parseVariable(StringRef &Str,
                                                     const SourceMgr &SM);
   /// Parses \p Expr for the name of a numeric variable to be defined at line
   /// \p LineNumber. \returns a pointer to the class instance representing that
   /// variable, creating it if needed, or an error holding a diagnostic against
   /// \p SM should defining such a variable be invalid.
   static Expected<FileCheckNumericVariable *>
   parseNumericVariableDefinition(StringRef &Expr,
                                  FileCheckPatternContext *Context,
                                  size_t LineNumber, const SourceMgr &SM);
-  /// Parses \p Expr for a numeric substitution block. \returns the class
-  /// representing the AST of the expression whose value must be substituted,
-  /// or an error holding a diagnostic against \p SM if parsing fails. If
-  /// substitution was successful, sets \p DefinedNumericVariable to point to
-  /// the class representing the numeric variable defined in this numeric
+  /// Parses \p Expr for a numeric substitution block. Parameter
+  /// \p IsLegacyLineExpr indicates whether \p Expr should be a legacy @LINE
+  /// expression. \returns a pointer to the class instance representing the AST
+  /// of the expression whose value must be substituted, or an error holding a
+  /// diagnostic against \p SM if parsing fails. If substitution was
+  /// successful, sets \p DefinedNumericVariable to point to the class
+  /// representing the numeric variable being defined in this numeric
   /// substitution block, or None if this block does not define any variable.
-  Expected<FileCheckExpression *> parseNumericSubstitutionBlock(
+  Expected<std::unique_ptr<FileCheckExpressionAST>>
+  parseNumericSubstitutionBlock(
       StringRef Expr,
       Optional<FileCheckNumericVariable *> &DefinedNumericVariable,
-      const SourceMgr &SM) const;
+      bool IsLegacyLineExpr, const SourceMgr &SM) const;
   /// Parses the pattern in \p PatternStr and initializes this FileCheckPattern
   /// instance accordingly.
   ///
   /// \p Prefix provides which prefix is being matched, \p Req describes the
   /// global options that influence the parsing such as whitespace
   /// canonicalization, \p SM provides the SourceMgr used for error reports.
   /// \returns true in case of an error, false otherwise.
   bool parsePattern(StringRef PatternStr, StringRef Prefix, SourceMgr &SM,
                     const FileCheckRequest &Req);
   /// Matches the pattern string against the input buffer \p Buffer
   ///
   /// \returns the position that is matched or an error indicating why matching
   /// failed. If there is a match, updates \p MatchLen with the size of the
   /// matched string.
   ///
   /// The GlobalVariableTable StringMap in the FileCheckPatternContext class
   /// instance provides the current values of FileCheck string variables and
   /// is updated if this match defines new values. Likewise, the
   /// GlobalNumericVariableTable StringMap in the same class provides the
   /// current values of FileCheck numeric variables and is updated if this
   /// match defines new numeric values.
   Expected<size_t> match(StringRef Buffer, size_t &MatchLen,
                          const SourceMgr &SM) const;
   /// Prints the value of successful substitutions or the name of the undefined
-  /// string or numeric variable preventing a successful substitution.
+  /// string or numeric variables preventing a successful substitution.
   void printSubstitutions(const SourceMgr &SM, StringRef Buffer,
                           SMRange MatchRange = None) const;
   void printFuzzyMatch(const SourceMgr &SM, StringRef Buffer,
                        std::vector<FileCheckDiag> *Diags) const;
 
   bool hasVariable() const {
     return !(Substitutions.empty() && VariableDefs.empty());
   }
private:
   /// Finds the closing sequence of a regex variable usage or definition.
   ///
   /// \p Str has to point in the beginning of the definition (right after the
   /// opening sequence). \p SM holds the SourceMgr used for error repporting.
   ///  \returns the offset of the closing sequence within Str, or npos if it
   /// was not found.
   size_t FindRegexVarEnd(StringRef Str, SourceMgr &SM);
 
-  /// Parses \p Expr for the use of a numeric variable. \returns the pointer to
-  /// the class instance representing that variable if successful, or an error
+  /// Parses \p Name as a (pseudo if \p IsPseudo is true) numeric variable use.
+  /// \returns the pointer to the class instance representing that variable if
+  /// successful, or an error holding a diagnostic against \p SM otherwise.
+  Expected<std::unique_ptr<FileCheckNumericVariableUse>>
+  parseNumericVariableUse(StringRef Name, bool IsPseudo,
+                          const SourceMgr &SM) const;
+  enum class AllowedOperand { LineVar, Literal, Any };
+  /// Parses \p Expr for use of a numeric operand. Accepts both literal values
+  /// and numeric variables, depending on the value of \p AO. \returns the
+  /// class representing that operand in the AST of the expression or an error
   /// holding a diagnostic against \p SM otherwise.
-  Expected<FileCheckNumericVariable *>
-  parseNumericVariableUse(StringRef &Expr, const SourceMgr &SM) const;
-  /// Parses \p Expr for a binary operation.
-  /// \returns the class representing the binary operation of the expression,
-  /// or an error holding a diagnostic against \p SM otherwise.
-  Expected<FileCheckExpression *> parseBinop(StringRef &Expr,
-                                             const SourceMgr &SM) const;
+  Expected<std::unique_ptr<FileCheckExpressionAST>>
+  parseNumericOperand(StringRef &Expr, AllowedOperand AO,
+  /// Parses \p Expr for a binary operation. The left operand of this binary
+  /// operation is given in \p LeftOp and \p IsLegacyLineExpr indicates whether
+  /// we are parsing a legacy @LINE expression. \returns the class representing
+  /// the binary operation in the AST of the expression, or an error holding a
+  /// diagnostic against \p SM otherwise.
+  Expected<std::unique_ptr<FileCheckExpressionAST>>
+  parseBinop(StringRef &Expr, std::unique_ptr<FileCheckExpressionAST> LeftOp,
+             bool IsLegacyLineExpr, const SourceMgr &SM) const;
 };
 
 //===----------------------------------------------------------------------===//
 /// Summary of a FileCheck diagnostic.
 //===----------------------------------------------------------------------===//
 
 struct FileCheckDiag {
   /// What is the FileCheck directive for this diagnostic?

llvm/trunk/lib/Support/FileCheck.cpp

 }
 
 void FileCheckNumericVariable::clearValue() {
   if (!Value)
     return;
   Value = None;
 }
 
-Expected<uint64_t> FileCheckExpression::eval() const {
-  assert(LeftOp && "Evaluating an empty expression");
-  Optional<uint64_t> LeftOpValue = LeftOp->getValue();
-  // Variable is undefined.
-  if (!LeftOpValue)
-    return make_error<FileCheckUndefVarError>(LeftOp->getName());
-  return EvalBinop(*LeftOpValue, RightOp);
+Expected<uint64_t> FileCheckNumericVariableUse::eval() const {
+  Optional<uint64_t> Value = NumericVariable->getValue();
+  if (Value)
+    return *Value;
+  return make_error<FileCheckUndefVarError>(Name);
+}
+
+Expected<uint64_t> FileCheckASTBinop::eval() const {
+  Expected<uint64_t> LeftOp = LeftOperand->eval();
+  Expected<uint64_t> RightOp = RightOperand->eval();
+
+  // Bubble up any error (e.g. undefined variables) in the recursive
+  // evaluation.
+  if (!LeftOp || !RightOp) {
+    Error Err = Error::success();
+    if (!LeftOp)
+      Err = joinErrors(std::move(Err), LeftOp.takeError());
+    if (!RightOp)
+      Err = joinErrors(std::move(Err), RightOp.takeError());
+    return std::move(Err);
+  }
+
+  return EvalBinop(*LeftOp, *RightOp);
 }
 
 Expected<std::string> FileCheckNumericSubstitution::getResult() const {
-  Expected<uint64_t> EvaluatedValue = Expression->eval();
+  Expected<uint64_t> EvaluatedValue = ExpressionAST->eval();
   if (!EvaluatedValue)
     return EvaluatedValue.takeError();
   return utostr(*EvaluatedValue);
 }
 
 Expected<std::string> FileCheckStringSubstitution::getResult() const {
   // Look up the value and escape it so that we can put it into the regex.
   Expected<StringRef> VarVal = Context->getPatternVarValue(FromStr);
   if (!VarVal)
     return VarVal.takeError();
   return Regex::escape(*VarVal);
 }
 
 bool FileCheckPattern::isValidVarNameStart(char C) {
   return C == '_' || isalpha(C);
 }
 
-Expected<StringRef> FileCheckPattern::parseVariable(StringRef &Str,
-                                                    bool &IsPseudo,
-                                                    const SourceMgr &SM) {
+Expected<FileCheckPattern::VariableProperties>
+FileCheckPattern::parseVariable(StringRef &Str, const SourceMgr &SM) {
   if (Str.empty())
     return FileCheckErrorDiagnostic::get(SM, Str, "empty variable name");
 
   bool ParsedOneChar = false;
   unsigned I = 0;
-  IsPseudo = Str[0] == '@';
+  bool IsPseudo = Str[0] == '@';
 
   // Global vars start with '$'.
   if (Str[0] == '$' || IsPseudo)
     ++I;
 
   for (unsigned E = Str.size(); I != E; ++I) {
     if (!ParsedOneChar && !isValidVarNameStart(Str[I]))
       return FileCheckErrorDiagnostic::get(SM, Str, "invalid variable name");
 
     // Variable names are composed of alphanumeric characters and underscores.
     if (Str[I] != '_' && !isalnum(Str[I]))
       break;
     ParsedOneChar = true;
   }
 
   StringRef Name = Str.take_front(I);
   Str = Str.substr(I);
-  return Name;
+  return VariableProperties {Name, IsPseudo};
 }
 
 // StringRef holding all characters considered as horizontal whitespaces by
 // FileCheck input canonicalization.
 StringRef SpaceChars = " \t";
 
 // Parsing helper function that strips the first character in S and returns it.
 static char popFront(StringRef &S) {
   char C = S.front();
   S = S.drop_front();
   return C;
 }
 
 char FileCheckUndefVarError::ID = 0;
 char FileCheckErrorDiagnostic::ID = 0;
 char FileCheckNotFoundError::ID = 0;
 
 Expected<FileCheckNumericVariable *>
 FileCheckPattern::parseNumericVariableDefinition(
     StringRef &Expr, FileCheckPatternContext *Context, size_t LineNumber,
     const SourceMgr &SM) {
-  bool IsPseudo;
-  Expected<StringRef> ParseVarResult = parseVariable(Expr, IsPseudo, SM);
+  Expected<VariableProperties> ParseVarResult = parseVariable(Expr, SM);
   if (!ParseVarResult)
     return ParseVarResult.takeError();
-  StringRef Name = *ParseVarResult;
+  StringRef Name = ParseVarResult->Name;
 
-  if (IsPseudo)
+  if (ParseVarResult->IsPseudo)
     return FileCheckErrorDiagnostic::get(
         SM, Name, "definition of pseudo numeric variable unsupported");
 
   // Detect collisions between string and numeric variables when the latter
   // is created later than the former.
   if (Context->DefinedVariableTable.find(Name) !=
       Context->DefinedVariableTable.end())
     return FileCheckErrorDiagnostic::get(
FileCheckPattern::parseNumericVariableDefinition(
   if (VarTableIter != Context->GlobalNumericVariableTable.end())
     DefinedNumericVariable = VarTableIter->second;
   else
     DefinedNumericVariable = Context->makeNumericVariable(LineNumber, Name);
 
   return DefinedNumericVariable;
 }
 
-Expected<FileCheckNumericVariable *>
-FileCheckPattern::parseNumericVariableUse(StringRef &Expr,
+Expected<std::unique_ptr<FileCheckNumericVariableUse>>
+FileCheckPattern::parseNumericVariableUse(StringRef Name, bool IsPseudo,
                                           const SourceMgr &SM) const {
-  bool IsPseudo;
-  Expected<StringRef> ParseVarResult = parseVariable(Expr, IsPseudo, SM);
-  if (!ParseVarResult)
-    return ParseVarResult.takeError();
-  StringRef Name = *ParseVarResult;
-
   if (IsPseudo && !Name.equals("@LINE"))
     return FileCheckErrorDiagnostic::get(
         SM, Name, "invalid pseudo numeric variable '" + Name + "'");
 
   // Numeric variable definitions and uses are parsed in the order in which
   // they appear in the CHECK patterns. For each definition, the pointer to the
   // class instance of the corresponding numeric variable definition is stored
   // in GlobalNumericVariableTable in parsePattern. Therefore, if the pointer
else {
     Context->GlobalNumericVariableTable[Name] = NumericVariable;
   }
 
   if (!IsPseudo && NumericVariable->getDefLineNumber() == LineNumber)
     return FileCheckErrorDiagnostic::get(
         SM, Name,
         "numeric variable '" + Name + "' defined on the same line as used");
 
-  return NumericVariable;
+  return llvm::make_unique<FileCheckNumericVariableUse>(Name, NumericVariable);
+}
+
+Expected<std::unique_ptr<FileCheckExpressionAST>>
+FileCheckPattern::parseNumericOperand(StringRef &Expr, AllowedOperand AO,
+                                      const SourceMgr &SM) const {
+  if (AO == AllowedOperand::LineVar || AO == AllowedOperand::Any) {
+    // Try to parse as a numeric variable use.
+    Expected<FileCheckPattern::VariableProperties> ParseVarResult =
+        parseVariable(Expr, SM);
+    if (ParseVarResult)
+      return parseNumericVariableUse(ParseVarResult->Name,
+                                     ParseVarResult->IsPseudo, SM);
+    if (AO == AllowedOperand::LineVar)
+      return ParseVarResult.takeError();
+    // Ignore the error and retry parsing as a literal.
+    consumeError(ParseVarResult.takeError());
+  }
+
+  // Otherwise, parse it as a literal.
+  uint64_t LiteralValue;
+  if (!Expr.consumeInteger(/*Radix=*/10, LiteralValue))
+    return llvm::make_unique<FileCheckExpressionLiteral>(LiteralValue);
+
+  return FileCheckErrorDiagnostic::get(SM, Expr,
+                                       "invalid operand format '" + Expr + "'");
 }
 
 static uint64_t add(uint64_t LeftOp, uint64_t RightOp) {
   return LeftOp + RightOp;
 }
 
 static uint64_t sub(uint64_t LeftOp, uint64_t RightOp) {
   return LeftOp - RightOp;
 }
 
-Expected<FileCheckExpression *>
-FileCheckPattern::parseBinop(StringRef &Expr, const SourceMgr &SM) const {
-  Expected<FileCheckNumericVariable *> LeftParseResult =
-      parseNumericVariableUse(Expr, SM);
-  if (!LeftParseResult) {
-    return LeftParseResult.takeError();
-  }
-  FileCheckNumericVariable *LeftOp = *LeftParseResult;
+Expected<std::unique_ptr<FileCheckExpressionAST>>
+FileCheckPattern::parseBinop(StringRef &Expr,
+                             std::unique_ptr<FileCheckExpressionAST> LeftOp,
+                             bool IsLegacyLineExpr, const SourceMgr &SM) const {
+  Expr = Expr.ltrim(SpaceChars);
+  if (Expr.empty())
+    return std::move(LeftOp);
 
   // Check if this is a supported operation and select a function to perform
   // it.
-  Expr = Expr.ltrim(SpaceChars);
-  if (Expr.empty())
-    return Context->makeExpression(add, LeftOp, 0);
   SMLoc OpLoc = SMLoc::getFromPointer(Expr.data());
   char Operator = popFront(Expr);
   binop_eval_t EvalBinop;
   switch (Operator) {
   case '+':
     EvalBinop = add;
     break;
   case '-':
     EvalBinop = sub;
     break;
   default:
     return FileCheckErrorDiagnostic::get(
         SM, OpLoc, Twine("unsupported operation '") + Twine(Operator) + "'");
   }
 
   // Parse right operand.
   Expr = Expr.ltrim(SpaceChars);
   if (Expr.empty())
     return FileCheckErrorDiagnostic::get(SM, Expr,
                                          "missing operand in expression");
-  uint64_t RightOp;
-  if (Expr.consumeInteger(10, RightOp))
-    return FileCheckErrorDiagnostic::get(
-        SM, Expr, "invalid offset in expression '" + Expr + "'");
-  Expr = Expr.ltrim(SpaceChars);
-  if (!Expr.empty())
-    return FileCheckErrorDiagnostic::get(
-        SM, Expr, "unexpected characters at end of expression '" + Expr + "'");
+  // The second operand in a legacy @LINE expression is always a literal.
+  AllowedOperand AO =
+      IsLegacyLineExpr ? AllowedOperand::Literal : AllowedOperand::Any;
+  Expected<std::unique_ptr<FileCheckExpressionAST>> RightOpResult =
+      parseNumericOperand(Expr, AO, SM);
+  if (!RightOpResult)
+    return RightOpResult;
 
-  return Context->makeExpression(EvalBinop, LeftOp, RightOp);
+  Expr = Expr.ltrim(SpaceChars);
+  return llvm::make_unique<FileCheckASTBinop>(EvalBinop, std::move(LeftOp),
+                                              std::move(*RightOpResult));
 }
 
-Expected<FileCheckExpression *> FileCheckPattern::parseNumericSubstitutionBlock(
+Expected<std::unique_ptr<FileCheckExpressionAST>>
+FileCheckPattern::parseNumericSubstitutionBlock(
     StringRef Expr,
     Optional<FileCheckNumericVariable *> &DefinedNumericVariable,
-    const SourceMgr &SM) const {
+    bool IsLegacyLineExpr, const SourceMgr &SM) const {
   // Parse the numeric variable definition.
   DefinedNumericVariable = None;
   size_t DefEnd = Expr.find(':');
   if (DefEnd != StringRef::npos) {
     StringRef DefExpr = Expr.substr(0, DefEnd);
     StringRef UseExpr = Expr.substr(DefEnd + 1);
 
     UseExpr = UseExpr.ltrim(SpaceChars);
     if (!UseExpr.empty())
       return FileCheckErrorDiagnostic::get(
           SM, UseExpr,
           "unexpected string after variable definition: '" + UseExpr + "'");
 
     DefExpr = DefExpr.ltrim(SpaceChars);
     Expected<FileCheckNumericVariable *> ParseResult =
         parseNumericVariableDefinition(DefExpr, Context, LineNumber, SM);
     if (!ParseResult)
       return ParseResult.takeError();
     DefinedNumericVariable = *ParseResult;
 
-    return Context->makeExpression(add, nullptr, 0);
+    return nullptr;
   }
 
   // Parse the expression itself.
   Expr = Expr.ltrim(SpaceChars);
-  return parseBinop(Expr, SM);
+  // The first operand in a legacy @LINE expression is always the @LINE pseudo
+  // variable.
+  AllowedOperand AO =
+      IsLegacyLineExpr ? AllowedOperand::LineVar : AllowedOperand::Any;
+  Expected<std::unique_ptr<FileCheckExpressionAST>> ParseResult =
+      parseNumericOperand(Expr, AO, SM);
+  while (ParseResult && !Expr.empty()) {
+    ParseResult =
+        parseBinop(Expr, std::move(*ParseResult), IsLegacyLineExpr, SM);
+    // Legacy @LINE expressions only allow 2 operands.
+    if (ParseResult && IsLegacyLineExpr && !Expr.empty())
+      return FileCheckErrorDiagnostic::get(
+          SM, Expr,
+          "unexpected characters at end of expression '" + Expr + "'");
+  }
+  if (!ParseResult)
+    return ParseResult;
+  return std::move(*ParseResult);
 }
 
 bool FileCheckPattern::parsePattern(StringRef PatternStr, StringRef Prefix,
                                     SourceMgr &SM,
                                     const FileCheckRequest &Req) {
   bool MatchFullLinesHere = Req.MatchFullLines && CheckTy != Check::CheckNot;
 
   PatternLoc = SMLoc::getFromPointer(PatternStr.data());
if (PatternStr.startswith("[[")) {
         return true;
       }
       // Strip the substitution block we are parsing. End points to the start
       // of the "]]" closing the expression so account for it in computing the
       // index of the first unparsed character.
       PatternStr = UnparsedPatternStr.substr(End + 2);
 
       bool IsDefinition = false;
+      // Whether the substitution block is a legacy use of @LINE with string
+      // substitution block syntax.
+      bool IsLegacyLineExpr = false;
       StringRef DefName;
       StringRef SubstStr;
       StringRef MatchRegexp;
       size_t SubstInsertIdx = RegExStr.size();
 
-      // Parse string variable or legacy expression.
+      // Parse string variable or legacy @LINE expression.
       if (!IsNumBlock) {
         size_t VarEndIdx = MatchStr.find(":");
         size_t SpacePos = MatchStr.substr(0, VarEndIdx).find_first_of(" \t");
         if (SpacePos != StringRef::npos) {
           SM.PrintMessage(SMLoc::getFromPointer(MatchStr.data() + SpacePos),
                           SourceMgr::DK_Error, "unexpected whitespace");
           return true;
         }
 
         // Get the name (e.g. "foo") and verify it is well formed.
-        bool IsPseudo;
         StringRef OrigMatchStr = MatchStr;
-        Expected<StringRef> ParseVarResult =
-            parseVariable(MatchStr, IsPseudo, SM);
+        Expected<FileCheckPattern::VariableProperties> ParseVarResult =
+            parseVariable(MatchStr, SM);
         if (!ParseVarResult) {
           logAllUnhandledErrors(ParseVarResult.takeError(), errs());
           return true;
         }
-        StringRef Name = *ParseVarResult;
+        StringRef Name = ParseVarResult->Name;
+        bool IsPseudo = ParseVarResult->IsPseudo;
 
         IsDefinition = (VarEndIdx != StringRef::npos);
         if (IsDefinition) {
           if ((IsPseudo || !MatchStr.consume_front(":"))) {
             SM.PrintMessage(SMLoc::getFromPointer(Name.data()),
                             SourceMgr::DK_Error,
                             "invalid name in string variable definition");
             return true;
           }
 
           // Detect collisions between string and numeric variables when the
           // former is created later than the latter.
           if (Context->GlobalNumericVariableTable.find(Name) !=
               Context->GlobalNumericVariableTable.end()) {
             SM.PrintMessage(
                 SMLoc::getFromPointer(Name.data()), SourceMgr::DK_Error,
                 "numeric variable with name '" + Name + "' already exists");
             return true;
           }
           DefName = Name;
           MatchRegexp = MatchStr;
         } else {
           if (IsPseudo) {
             MatchStr = OrigMatchStr;
-            IsNumBlock = true;
+            IsLegacyLineExpr = IsNumBlock = true;
           } else
             SubstStr = Name;
         }
       }
 
       // Parse numeric substitution block.
-      FileCheckExpression *Expression;
+      std::unique_ptr<FileCheckExpressionAST> ExpressionAST;
       Optional<FileCheckNumericVariable *> DefinedNumericVariable;
       if (IsNumBlock) {
-        Expected<FileCheckExpression *> ParseResult =
-            parseNumericSubstitutionBlock(MatchStr, DefinedNumericVariable, SM);
+        Expected<std::unique_ptr<FileCheckExpressionAST>> ParseResult =
+            parseNumericSubstitutionBlock(MatchStr, DefinedNumericVariable,
+                                          IsLegacyLineExpr, SM);
         if (!ParseResult) {
           logAllUnhandledErrors(ParseResult.takeError(), errs());
           return true;
         }
-        Expression = *ParseResult;
+        ExpressionAST = std::move(*ParseResult);
         if (DefinedNumericVariable) {
           IsDefinition = true;
           DefName = (*DefinedNumericVariable)->getName();
           MatchRegexp = StringRef("[0-9]+");
         } else
           SubstStr = MatchStr;
       }
 
if (PatternStr.startswith("[[")) {
             return true;
           }
           AddBackrefToRegEx(CaptureParenGroup);
         } else {
           // Handle substitution of string variables ([[<var>]]) defined in
           // previous CHECK patterns, and substitution of expressions.
           FileCheckSubstitution *Substitution =
               IsNumBlock
-                  ? Context->makeNumericSubstitution(SubstStr, Expression,
-                                                     SubstInsertIdx)
+                  ? Context->makeNumericSubstitution(
+                        SubstStr, std::move(ExpressionAST), SubstInsertIdx)
                   : Context->makeStringSubstitution(SubstStr, SubstInsertIdx);
           Substitutions.push_back(Substitution);
         }
         continue;
       }
 
       // Handle variable definitions: [[<def>:(...)]] and
       // [[#(...)<def>:(...)]].
void FileCheckPattern::printSubstitutions(const SourceMgr &SM, StringRef Buffer,
   // Print what we know about substitutions.
   if (!Substitutions.empty()) {
     for (const auto &Substitution : Substitutions) {
       SmallString<256> Msg;
       raw_svector_ostream OS(Msg);
       Expected<std::string> MatchedValue = Substitution->getResult();
 
       // Substitution failed or is not known at match time, print the undefined
-      // variable it uses.
+      // variables it uses.
       if (!MatchedValue) {
         bool UndefSeen = false;
         handleAllErrors(MatchedValue.takeError(),
                         [](const FileCheckNotFoundError &E) {},
                         // Handled in PrintNoMatch().
                         [](const FileCheckErrorDiagnostic &E) {},
                         [&](const FileCheckUndefVarError &E) {
                           if (!UndefSeen) {
-                            OS << "uses undefined variable ";
+                            OS << "uses undefined variable(s):";
                             UndefSeen = true;
                           }
+                          OS << " ";
                           E.log(OS);
-                        },
-                        [](const ErrorInfoBase &E) {
-                          llvm_unreachable("Unexpected error");
                         });
       } else {
         // Substitution succeeded. Print substituted value.
         OS << "with \"";
         OS.write_escaped(Substitution->getFromString()) << "\" equal to \"";
         OS.write_escaped(*MatchedValue) << "\"";
       }
 

 FileCheckPatternContext::getPatternVarValue(StringRef VarName) {
   auto VarIter = GlobalVariableTable.find(VarName);
   if (VarIter == GlobalVariableTable.end())
     return make_error<FileCheckUndefVarError>(VarName);
 
   return VarIter->second;
 }
 
-FileCheckExpression *
-FileCheckPatternContext::makeExpression(binop_eval_t EvalBinop,
-                                        FileCheckNumericVariable *OperandLeft,
-                                        uint64_t OperandRight) {
-  Expressions.push_back(llvm::make_unique<FileCheckExpression>(
-      EvalBinop, OperandLeft, OperandRight));
-  return Expressions.back().get();
-}
-
 template <class... Types>
 FileCheckNumericVariable *
 FileCheckPatternContext::makeNumericVariable(Types... args) {
   NumericVariables.push_back(
       llvm::make_unique<FileCheckNumericVariable>(args...));
   return NumericVariables.back().get();
 }
 
 FileCheckSubstitution *
 FileCheckPatternContext::makeStringSubstitution(StringRef VarName,
                                                 size_t InsertIdx) {
   Substitutions.push_back(
       llvm::make_unique<FileCheckStringSubstitution>(this, VarName, InsertIdx));
   return Substitutions.back().get();
 }
 
 FileCheckSubstitution *FileCheckPatternContext::makeNumericSubstitution(
-    StringRef ExpressionStr, FileCheckExpression *Expression,
-    size_t InsertIdx) {
+    StringRef ExpressionStr,
+    std::unique_ptr<FileCheckExpressionAST> ExpressionAST, size_t InsertIdx) {
   Substitutions.push_back(llvm::make_unique<FileCheckNumericSubstitution>(
-      this, ExpressionStr, Expression, InsertIdx));
+      this, ExpressionStr, std::move(ExpressionAST), InsertIdx));
   return Substitutions.back().get();
 }
 
 size_t FileCheckPattern::FindRegexVarEnd(StringRef Str, SourceMgr &SM) {
   // Offset keeps track of the current offset within the input Str
   size_t Offset = 0;
   // [...] Nesting depth
   size_t BracketDepth = 0;
if (CmdlineDef[0] == '#') {
       // Record this variable definition.
       GlobalNumericVariableTable[DefinedNumericVariable->getName()] =
           DefinedNumericVariable;
     } else {
       // String variable definition.
       std::pair<StringRef, StringRef> CmdlineNameVal = CmdlineDef.split('=');
       StringRef CmdlineName = CmdlineNameVal.first;
       StringRef OrigCmdlineName = CmdlineName;
-      bool IsPseudo;
-      Expected<StringRef> ParseVarResult =
-          FileCheckPattern::parseVariable(CmdlineName, IsPseudo, SM);
+      Expected<FileCheckPattern::VariableProperties> ParseVarResult =
+          FileCheckPattern::parseVariable(CmdlineName, SM);
       if (!ParseVarResult) {
         Errs = joinErrors(std::move(Errs), ParseVarResult.takeError());
         continue;
       }
       // Check that CmdlineName does not denote a pseudo variable is only
       // composed of the parsed numeric variable. This catches cases like
       // "FOO+2" in a "FOO+2=10" definition.
-      if (IsPseudo || !CmdlineName.empty()) {
+      if (ParseVarResult->IsPseudo || !CmdlineName.empty()) {
         Errs = joinErrors(std::move(Errs),
                           FileCheckErrorDiagnostic::get(
                               SM, OrigCmdlineName,
                               "invalid name in string variable definition '" +
                                   OrigCmdlineName + "'"));
         continue;
       }
-      StringRef Name = *ParseVarResult;
+      StringRef Name = ParseVarResult->Name;
 
       // Detect collisions between string and numeric variables when the former
       // is created later than the latter.
       if (GlobalNumericVariableTable.find(Name) !=
           GlobalNumericVariableTable.end()) {
         Errs = joinErrors(std::move(Errs), FileCheckErrorDiagnostic::get(
                                                SM, Name,
                                                "numeric variable with name '" +

llvm/trunk/test/FileCheck/line-count.txt

 45
 46 BAD8: [[@LIN]]
 47 ERR8: line-count.txt:[[#@LINE-1]]:12: error: invalid pseudo numeric variable '@LIN'
 48
 49 BAD9: [[@LINE*2]]
 50 ERR9: line-count.txt:[[#@LINE-1]]:17: error: unsupported operation '*'
 51
 52 BAD10: [[@LINE-x]]
-53 ERR10: line-count.txt:[[#@LINE-1]]:19: error: invalid offset in expression 'x'
+53 ERR10: line-count.txt:[[#@LINE-1]]:19: error: invalid operand format 'x'
 54
 55 BAD11: [[@LINE-1x]]
 56 ERR11: line-count.txt:[[#@LINE-1]]:20: error: unexpected characters at end of expression 'x'
 57
 58 CHECK: [[#@LINE]] CHECK
 59 CHECK: [[# @LINE]] CHECK
 60 CHECK: [[# @LINE ]] CHECK
 61
 62 CHECK: [[#@LINE-1]]
 63 CHECK: [[# @LINE-1]] CHECK
 64 CHECK: [[# @LINE -1]] CHECK
 65 CHECK: [[# @LINE - 1]] CHECK
 66 CHECK: [[# @LINE - 1 ]] CHECK

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

 CHECK-NEXT: [[# VAR1 +1]]
 CHECK-NEXT: [[# VAR1 + 1]]
 CHECK-NEXT: [[# VAR1 + 1 ]]
 CHECK-NEXT: [[# VAR1-1]]
 CHECK-NEXT: [[# VAR1 -1]]
 CHECK-NEXT: [[# VAR1 - 1]]
 CHECK-NEXT: [[# VAR1 - 1 ]]
 
-; Numeric expressions using variables defined on the command-line and an
-; immediate interpreted as an unsigned value.
+; Numeric expressions using variables defined on other lines and an immediate
+; interpreted as an unsigned value.
 ; Note: 9223372036854775819 = 0x8000000000000000 + 11
 ;       9223372036854775808 = 0x8000000000000000
 USE UNSIGNED IMM
 9223372036854775819
 CHECK-LABEL: USE UNSIGNED IMM
 CHECK-NEXT: [[#VAR1+9223372036854775808]]
 
-; Numeric expression using undefined variable.
+; Numeric expressions using more than one variable defined on other lines.
+USE MULTI VAR
+31
+42
+CHECK-LABEL: USE MULTI VAR
+CHECK-NEXT: [[#VAR2:]]
+CHECK-NEXT: [[#VAR1+VAR2]]
+
+; Numeric expression using undefined variables.
 RUN: not FileCheck --check-prefix UNDEF-USE --input-file %s %s 2>&1 \
 RUN:   | FileCheck --strict-whitespace --check-prefix UNDEF-USE-MSG %s
 
 UNDEF VAR USE
 UNDEFVAR: 11
 UNDEF-USE-LABEL: UNDEF VAR USE
-UNDEF-USE-NEXT: UNDEFVAR: [[#UNDEFVAR]]
+UNDEF-USE-NEXT: UNDEFVAR: [[#UNDEFVAR1+UNDEFVAR2]]
 UNDEF-USE-MSG: numeric-expression.txt:[[#@LINE-1]]:17: error: {{U}}NDEF-USE-NEXT: expected string not found in input
-UNDEF-USE-MSG-NEXT: {{U}}NDEF-USE-NEXT: UNDEFVAR: {{\[\[#UNDEFVAR\]\]}}
+UNDEF-USE-MSG-NEXT: {{U}}NDEF-USE-NEXT: UNDEFVAR: {{\[\[#UNDEFVAR1\+UNDEFVAR2\]\]}}
 UNDEF-USE-MSG-NEXT: {{^                \^$}}
 UNDEF-USE-MSG-NEXT: numeric-expression.txt:[[#@LINE-6]]:1: note: scanning from here
 UNDEF-USE-MSG-NEXT: UNDEFVAR: 11
 UNDEF-USE-MSG-NEXT: {{^\^$}}
-UNDEF-USE-MSG-NEXT: numeric-expression.txt:[[#@LINE-9]]:1: note: uses undefined variable "UNDEFVAR"
+UNDEF-USE-MSG-NEXT: numeric-expression.txt:[[#@LINE-9]]:1: note: uses undefined variable(s): "UNDEFVAR1" "UNDEFVAR2"
 UNDEF-USE-MSG-NEXT: UNDEFVAR: 11
 UNDEF-USE-MSG-NEXT: {{^\^$}}
 
 ; Numeric expression with unsupported operator.
 RUN: not FileCheck -D#NUMVAR=10 --check-prefix INVAL-OP --input-file %s %s 2>&1 \
 RUN:   | FileCheck --strict-whitespace --check-prefix INVAL-OP-MSG %s
 
 INVALID OPERATOR

llvm/trunk/test/FileCheck/var-scope.txt

 local3
 global3
 LOCAL1: [[LOCAL]]3
 LOCAL2: local[[#LOCNUM+2]]
 LOCAL3: [[LOCAL]][[#LOCNUM+2]]
 GLOBAL: [[$GLOBAL]][[#$GLOBNUM+2]]
 
 ERRUNDEF: expected string not found in input
-ERRUNDEFLOCAL: uses undefined variable "LOCAL"
-ERRUNDEFLOCNUM: uses undefined variable "LOCNUM"
+ERRUNDEFLOCAL: uses undefined variable(s): "LOCAL"
+ERRUNDEFLOCNUM: uses undefined variable(s): "LOCNUM"

llvm/trunk/unittests/Support/FileCheckTest.cpp

 //===- llvm/unittest/Support/FileCheckTest.cpp - FileCheck tests --===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/FileCheck.h"
 #include "gtest/gtest.h"
+#include <unordered_set>
 
 using namespace llvm;
 namespace {
 
 class FileCheckTest : public ::testing::Test {};
 
+TEST_F(FileCheckTest, Literal) {
+  // Eval returns the literal's value.
+  FileCheckExpressionLiteral Ten(10);
+  Expected<uint64_t> Value = Ten.eval();
+  EXPECT_TRUE(bool(Value));
+  EXPECT_EQ(10U, *Value);
+
+  // Max value can be correctly represented.
+  FileCheckExpressionLiteral Max(std::numeric_limits<uint64_t>::max());
+  Value = Max.eval();
+  EXPECT_TRUE(bool(Value));
+  EXPECT_EQ(std::numeric_limits<uint64_t>::max(), *Value);
+}
+
+static std::string toString(const std::unordered_set<std::string> &Set) {
+  bool First = true;
+  std::string Str;
+  for (StringRef S : Set) {
+    Str += Twine(First ? "{" + S : ", " + S).str();
+    First = false;
+  }
+  Str += '}';
+  return Str;
+}
+
+static void
+expectUndefErrors(std::unordered_set<std::string> ExpectedUndefVarNames,
+                  Error Err) {
+  handleAllErrors(std::move(Err), [&](const FileCheckUndefVarError &E) {
+    ExpectedUndefVarNames.erase(E.getVarName());
+  });
+  EXPECT_TRUE(ExpectedUndefVarNames.empty()) << toString(ExpectedUndefVarNames);
+}
+
+static void expectUndefError(const Twine &ExpectedUndefVarName, Error Err) {
+  expectUndefErrors({ExpectedUndefVarName.str()}, std::move(Err));
+}
+
 TEST_F(FileCheckTest, NumericVariable) {
-  // Undefined variable: getValue fails, setValue does not trigger assert.
+  // Undefined variable: getValue and eval fail, error returned by eval holds
+  // the name of the undefined variable and setValue does not trigger assert.
   FileCheckNumericVariable FooVar = FileCheckNumericVariable(1, "FOO");
   EXPECT_EQ("FOO", FooVar.getName());
-  llvm::Optional<uint64_t> Value = FooVar.getValue();
-  EXPECT_FALSE(Value);
-  FooVar.clearValue();
+  FileCheckNumericVariableUse FooVarUse =
+      FileCheckNumericVariableUse("FOO", &FooVar);
+  EXPECT_FALSE(FooVar.getValue());
+  Expected<uint64_t> EvalResult = FooVarUse.eval();
+  EXPECT_FALSE(EvalResult);
+  expectUndefError("FOO", EvalResult.takeError());
   FooVar.setValue(42);
 
-  // Defined variable: getValue returns value set.
-  Value = FooVar.getValue();
-  EXPECT_TRUE(Value);
+  // Defined variable: getValue and eval return value set.
+  Optional<uint64_t> Value = FooVar.getValue();
+  EXPECT_TRUE(bool(Value));
   EXPECT_EQ(42U, *Value);
+  EvalResult = FooVarUse.eval();
+  EXPECT_TRUE(bool(EvalResult));
+  EXPECT_EQ(42U, *EvalResult);
 
-  // Clearing variable: getValue fails.
+  // Clearing variable: getValue and eval fail. Error returned by eval holds
+  // the name of the cleared variable.
   FooVar.clearValue();
   Value = FooVar.getValue();
   EXPECT_FALSE(Value);
+  EvalResult = FooVarUse.eval();
+  EXPECT_FALSE(EvalResult);
+  expectUndefError("FOO", EvalResult.takeError());
 }
 
 uint64_t doAdd(uint64_t OpL, uint64_t OpR) { return OpL + OpR; }
 
-static void expectUndefError(const Twine &ExpectedStr, Error Err) {
-  handleAllErrors(std::move(Err), [&](const FileCheckUndefVarError &E) {
-    EXPECT_EQ(ExpectedStr.str(), E.getVarName());
-  });
-}
-
-TEST_F(FileCheckTest, Expression) {
+TEST_F(FileCheckTest, Binop) {
   FileCheckNumericVariable FooVar = FileCheckNumericVariable("FOO", 42);
-  FileCheckExpression Expression = FileCheckExpression(doAdd, &FooVar, 18);
+  std::unique_ptr<FileCheckNumericVariableUse> FooVarUse =
+      llvm::make_unique<FileCheckNumericVariableUse>("FOO", &FooVar);
+  FileCheckNumericVariable BarVar = FileCheckNumericVariable("BAR", 18);
+  std::unique_ptr<FileCheckNumericVariableUse> BarVarUse =
+      llvm::make_unique<FileCheckNumericVariableUse>("BAR", &BarVar);
+  FileCheckASTBinop Binop =
+      FileCheckASTBinop(doAdd, std::move(FooVarUse), std::move(BarVarUse));
 
   // Defined variable: eval returns right value.
-  Expected<uint64_t> Value = Expression.eval();
+  Expected<uint64_t> Value = Binop.eval();
   EXPECT_TRUE(bool(Value));
   EXPECT_EQ(60U, *Value);
 
-  // Undefined variable: eval fails, undefined variable returned. We call
-  // getUndefVarName first to check that it can be called without calling
-  // eval() first.
+  // 1 undefined variable: eval fails, error contains name of undefined
+  // variable.
   FooVar.clearValue();
-  Error EvalError = Expression.eval().takeError();
-  EXPECT_TRUE(errorToBool(std::move(EvalError)));
-  expectUndefError("FOO", std::move(EvalError));
+  Value = Binop.eval();
+  EXPECT_FALSE(Value);
+  expectUndefError("FOO", Value.takeError());
+
+  // 2 undefined variables: eval fails, error contains names of all undefined
+  // variables.
+  BarVar.clearValue();
+  Value = Binop.eval();
+  EXPECT_FALSE(Value);
+  expectUndefErrors({"FOO", "BAR"}, Value.takeError());
 }
 
 TEST_F(FileCheckTest, ValidVarNameStart) {
   EXPECT_TRUE(FileCheckPattern::isValidVarNameStart('a'));
   EXPECT_TRUE(FileCheckPattern::isValidVarNameStart('G'));
   EXPECT_TRUE(FileCheckPattern::isValidVarNameStart('_'));
   EXPECT_FALSE(FileCheckPattern::isValidVarNameStart('2'));
   EXPECT_FALSE(FileCheckPattern::isValidVarNameStart('$'));
static StringRef bufferize(SourceMgr &SM, StringRef Str) {
   SM.AddNewSourceBuffer(std::move(Buffer), SMLoc());
   return StrBufferRef;
 }
 
 TEST_F(FileCheckTest, ParseVar) {
   SourceMgr SM;
   StringRef OrigVarName = bufferize(SM, "GoodVar42");
   StringRef VarName = OrigVarName;
-  bool IsPseudo = true;
-  Expected<StringRef> ParsedName =
-      FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
-  EXPECT_TRUE(bool(ParsedName));
-  EXPECT_EQ(*ParsedName, OrigVarName);
+  Expected<FileCheckPattern::VariableProperties> ParsedVarResult =
+      FileCheckPattern::parseVariable(VarName, SM);
+  EXPECT_TRUE(bool(ParsedVarResult));
+  EXPECT_EQ(ParsedVarResult->Name, OrigVarName);
   EXPECT_TRUE(VarName.empty());
-  EXPECT_FALSE(IsPseudo);
+  EXPECT_FALSE(ParsedVarResult->IsPseudo);
 
   VarName = OrigVarName = bufferize(SM, "$GoodGlobalVar");
-  IsPseudo = true;
-  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
-  EXPECT_TRUE(bool(ParsedName));
-  EXPECT_EQ(*ParsedName, OrigVarName);
+  ParsedVarResult = FileCheckPattern::parseVariable(VarName, SM);
+  EXPECT_TRUE(bool(ParsedVarResult));
+  EXPECT_EQ(ParsedVarResult->Name, OrigVarName);
   EXPECT_TRUE(VarName.empty());
-  EXPECT_FALSE(IsPseudo);
+  EXPECT_FALSE(ParsedVarResult->IsPseudo);
 
   VarName = OrigVarName = bufferize(SM, "@GoodPseudoVar");
-  IsPseudo = true;
-  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
-  EXPECT_TRUE(bool(ParsedName));
-  EXPECT_EQ(*ParsedName, OrigVarName);
+  ParsedVarResult = FileCheckPattern::parseVariable(VarName, SM);
+  EXPECT_TRUE(bool(ParsedVarResult));
+  EXPECT_EQ(ParsedVarResult->Name, OrigVarName);
   EXPECT_TRUE(VarName.empty());
-  EXPECT_TRUE(IsPseudo);
+  EXPECT_TRUE(ParsedVarResult->IsPseudo);
 
   VarName = bufferize(SM, "42BadVar");
-  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
-  EXPECT_TRUE(errorToBool(ParsedName.takeError()));
+  ParsedVarResult = FileCheckPattern::parseVariable(VarName, SM);
+  EXPECT_TRUE(errorToBool(ParsedVarResult.takeError()));
 
   VarName = bufferize(SM, "$@");
-  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
-  EXPECT_TRUE(errorToBool(ParsedName.takeError()));
+  ParsedVarResult = FileCheckPattern::parseVariable(VarName, SM);
+  EXPECT_TRUE(errorToBool(ParsedVarResult.takeError()));
 
   VarName = OrigVarName = bufferize(SM, "B@dVar");
-  IsPseudo = true;
-  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
-  EXPECT_TRUE(bool(ParsedName));
+  ParsedVarResult = FileCheckPattern::parseVariable(VarName, SM);
+  EXPECT_TRUE(bool(ParsedVarResult));
   EXPECT_EQ(VarName, OrigVarName.substr(1));
-  EXPECT_EQ(*ParsedName, "B");
-  EXPECT_FALSE(IsPseudo);
+  EXPECT_EQ(ParsedVarResult->Name, "B");
+  EXPECT_FALSE(ParsedVarResult->IsPseudo);
 
   VarName = OrigVarName = bufferize(SM, "B$dVar");
-  IsPseudo = true;
-  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
-  EXPECT_TRUE(bool(ParsedName));
+  ParsedVarResult = FileCheckPattern::parseVariable(VarName, SM);
+  EXPECT_TRUE(bool(ParsedVarResult));
   EXPECT_EQ(VarName, OrigVarName.substr(1));
-  EXPECT_EQ(*ParsedName, "B");
-  EXPECT_FALSE(IsPseudo);
+  EXPECT_EQ(ParsedVarResult->Name, "B");
+  EXPECT_FALSE(ParsedVarResult->IsPseudo);
 
   VarName = bufferize(SM, "BadVar+");
-  IsPseudo = true;
-  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
-  EXPECT_TRUE(bool(ParsedName));
+  ParsedVarResult = FileCheckPattern::parseVariable(VarName, SM);
+  EXPECT_TRUE(bool(ParsedVarResult));
   EXPECT_EQ(VarName, "+");
-  EXPECT_EQ(*ParsedName, "BadVar");
-  EXPECT_FALSE(IsPseudo);
+  EXPECT_EQ(ParsedVarResult->Name, "BadVar");
+  EXPECT_FALSE(ParsedVarResult->IsPseudo);
 
   VarName = bufferize(SM, "BadVar-");
-  IsPseudo = true;
-  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
-  EXPECT_TRUE(bool(ParsedName));
+  ParsedVarResult = FileCheckPattern::parseVariable(VarName, SM);
+  EXPECT_TRUE(bool(ParsedVarResult));
   EXPECT_EQ(VarName, "-");
-  EXPECT_EQ(*ParsedName, "BadVar");
-  EXPECT_FALSE(IsPseudo);
+  EXPECT_EQ(ParsedVarResult->Name, "BadVar");
+  EXPECT_FALSE(ParsedVarResult->IsPseudo);
 
   VarName = bufferize(SM, "BadVar:");
-  IsPseudo = true;
-  ParsedName = FileCheckPattern::parseVariable(VarName, IsPseudo, SM);
-  EXPECT_TRUE(bool(ParsedName));
+  ParsedVarResult = FileCheckPattern::parseVariable(VarName, SM);
+  EXPECT_TRUE(bool(ParsedVarResult));
   EXPECT_EQ(VarName, ":");
-  EXPECT_EQ(*ParsedName, "BadVar");
-  EXPECT_FALSE(IsPseudo);
+  EXPECT_EQ(ParsedVarResult->Name, "BadVar");
+  EXPECT_FALSE(ParsedVarResult->IsPseudo);
 }
 
 class PatternTester {
 private:
   size_t LineNumber = 1;
   SourceMgr SM;
   FileCheckRequest Req;
   FileCheckPatternContext Context;
return errorToBool(FileCheckPattern::parseNumericVariableDefinition(
                            ExprBufferRef, &Context, LineNumber, SM)
                            .takeError());
   }
 
   bool parseSubstExpect(StringRef Expr) {
     StringRef ExprBufferRef = bufferize(SM, Expr);
     Optional<FileCheckNumericVariable *> DefinedNumericVariable;
     return errorToBool(P.parseNumericSubstitutionBlock(
-                            ExprBufferRef, DefinedNumericVariable, SM)
+                            ExprBufferRef, DefinedNumericVariable, false, SM)
                            .takeError());
   }
 
   bool parsePatternExpect(StringRef Pattern) {
     StringRef PatBufferRef = bufferize(SM, Pattern);
     return P.parsePattern(PatBufferRef, "CHECK", SM, Req);
   }
 
TEST_F(FileCheckTest, ParseExpr) {
   EXPECT_TRUE(Tester.parseSubstExpect("LINE1VAR"));
 
   // Unsupported operator.
   EXPECT_TRUE(Tester.parseSubstExpect("@LINE/2"));
 
   // Missing offset operand.
   EXPECT_TRUE(Tester.parseSubstExpect("@LINE+"));
 
-  // Cannot parse offset operand.
-  EXPECT_TRUE(Tester.parseSubstExpect("@LINE+x"));
-
-  // Unexpected string at end of numeric expression.
-  EXPECT_TRUE(Tester.parseSubstExpect("@LINE+5x"));
-
   // Valid expression.
   EXPECT_FALSE(Tester.parseSubstExpect("@LINE+5"));
   EXPECT_FALSE(Tester.parseSubstExpect("FOO+4"));
+  Tester.initNextPattern();
+  EXPECT_FALSE(Tester.parsePatternExpect("[[#FOO+FOO]]"));
+  EXPECT_FALSE(Tester.parsePatternExpect("[[#FOO+3-FOO]]"));
 }
 
 TEST_F(FileCheckTest, ParsePattern) {
   PatternTester Tester;
 
   // Space in pattern variable expression.
   EXPECT_TRUE(Tester.parsePatternExpect("[[ BAR]]"));
 
TEST_F(FileCheckTest, ParsePattern) {
 
   // Valid pattern variable definition.
   EXPECT_FALSE(Tester.parsePatternExpect("[[PAT:[0-9]+]]"));
 
   // Invalid numeric expressions.
   EXPECT_TRUE(Tester.parsePatternExpect("[[#42INVALID]]"));
   EXPECT_TRUE(Tester.parsePatternExpect("[[#@FOO]]"));
   EXPECT_TRUE(Tester.parsePatternExpect("[[#@LINE/2]]"));
-  EXPECT_TRUE(Tester.parsePatternExpect("[[#2+@LINE]]"));
   EXPECT_TRUE(Tester.parsePatternExpect("[[#YUP:@LINE]]"));
 
   // Valid numeric expressions and numeric variable definition.
   EXPECT_FALSE(Tester.parsePatternExpect("[[#FOO]]"));
   EXPECT_FALSE(Tester.parsePatternExpect("[[#@LINE+2]]"));
   EXPECT_FALSE(Tester.parsePatternExpect("[[#NUMVAR:]]"));
 }
 
TEST_F(FileCheckTest, Substitution) {
   Expected<std::string> SubstValue = StringSubstitution.getResult();
   EXPECT_FALSE(bool(SubstValue));
   expectUndefError("VAR404", SubstValue.takeError());
 
   // Substitutions of defined pseudo and non-pseudo numeric variables return
   // the right value.
   FileCheckNumericVariable LineVar = FileCheckNumericVariable("@LINE", 42);
   FileCheckNumericVariable NVar = FileCheckNumericVariable("N", 10);
-  FileCheckExpression LineExpression = FileCheckExpression(doAdd, &LineVar, 0);
-  FileCheckExpression NExpression = FileCheckExpression(doAdd, &NVar, 3);
-  FileCheckNumericSubstitution SubstitutionLine =
-      FileCheckNumericSubstitution(&Context, "@LINE", &LineExpression, 12);
+  auto LineVarUse =
+      llvm::make_unique<FileCheckNumericVariableUse>("@LINE", &LineVar);
+  auto NVarUse = llvm::make_unique<FileCheckNumericVariableUse>("N", &NVar);
+  FileCheckNumericSubstitution SubstitutionLine = FileCheckNumericSubstitution(
+      &Context, "@LINE", std::move(LineVarUse), 12);
   FileCheckNumericSubstitution SubstitutionN =
-      FileCheckNumericSubstitution(&Context, "N", &NExpression, 30);
-  Expected<std::string> Value = SubstitutionLine.getResult();
-  EXPECT_TRUE(bool(Value));
-  EXPECT_EQ("42", *Value);
-  Value = SubstitutionN.getResult();
-  EXPECT_TRUE(bool(Value));
-  EXPECT_EQ("13", *Value);
+      FileCheckNumericSubstitution(&Context, "N", std::move(NVarUse), 30);
+  SubstValue = SubstitutionLine.getResult();
+  EXPECT_TRUE(bool(SubstValue));
+  EXPECT_EQ("42", *SubstValue);
+  SubstValue = SubstitutionN.getResult();
+  EXPECT_TRUE(bool(SubstValue));
+  EXPECT_EQ("10", *SubstValue);
 
-  // Substitution of an undefined numeric variable fails.
+  // Substitution of an undefined numeric variable fails, error holds name of
+  // undefined variable.
   LineVar.clearValue();
-  SubstValue = SubstitutionLine.getResult().takeError();
+  SubstValue = SubstitutionLine.getResult();
   EXPECT_FALSE(bool(SubstValue));
   expectUndefError("@LINE", SubstValue.takeError());
   NVar.clearValue();
-  SubstValue = SubstitutionN.getResult().takeError();
+  SubstValue = SubstitutionN.getResult();
   EXPECT_FALSE(bool(SubstValue));
   expectUndefError("N", SubstValue.takeError());
 
   // Substitution of a defined string variable returns the right value.
   FileCheckPattern P = FileCheckPattern(Check::CheckPlain, &Context, 1);
   StringSubstitution = FileCheckStringSubstitution(&Context, "FOO", 42);
-  Value = StringSubstitution.getResult();
-  EXPECT_TRUE(bool(Value));
-  EXPECT_EQ("BAR", *Value);
+  SubstValue = StringSubstitution.getResult();
+  EXPECT_TRUE(bool(SubstValue));
+  EXPECT_EQ("BAR", *SubstValue);
 }
 
 TEST_F(FileCheckTest, FileCheckContext) {
   FileCheckPatternContext Cxt = FileCheckPatternContext();
   std::vector<std::string> GlobalDefines;
   SourceMgr SM;
 
   // Missing equal sign.
TEST_F(FileCheckTest, FileCheckContext) {
   // Check defined variables are present and undefined is absent.
   StringRef LocalVarStr = "LocalVar";
   StringRef LocalNumVarRef = bufferize(SM, "LocalNumVar");
   StringRef EmptyVarStr = "EmptyVar";
   StringRef UnknownVarStr = "UnknownVar";
   Expected<StringRef> LocalVar = Cxt.getPatternVarValue(LocalVarStr);
   FileCheckPattern P = FileCheckPattern(Check::CheckPlain, &Cxt, 1);
   Optional<FileCheckNumericVariable *> DefinedNumericVariable;
-  Expected<FileCheckExpression *> Expression = P.parseNumericSubstitutionBlock(
-      LocalNumVarRef, DefinedNumericVariable, SM);
-  Expected<StringRef> EmptyVar = Cxt.getPatternVarValue(EmptyVarStr);
-  Expected<StringRef> UnknownVar = Cxt.getPatternVarValue(UnknownVarStr);
+  Expected<std::unique_ptr<FileCheckExpressionAST>> ExpressionAST =
+      P.parseNumericSubstitutionBlock(LocalNumVarRef, DefinedNumericVariable,
+                                      /*IsLegacyLineExpr=*/false, SM);
   EXPECT_TRUE(bool(LocalVar));
   EXPECT_EQ(*LocalVar, "FOO");
-  EXPECT_TRUE(bool(Expression));
-  Expected<uint64_t> ExpressionVal = (*Expression)->eval();
+  Expected<StringRef> EmptyVar = Cxt.getPatternVarValue(EmptyVarStr);
+  Expected<StringRef> UnknownVar = Cxt.getPatternVarValue(UnknownVarStr);
+  EXPECT_TRUE(bool(ExpressionAST));
+  Expected<uint64_t> ExpressionVal = (*ExpressionAST)->eval();
   EXPECT_TRUE(bool(ExpressionVal));
   EXPECT_EQ(*ExpressionVal, 18U);
   EXPECT_TRUE(bool(EmptyVar));
   EXPECT_EQ(*EmptyVar, "");
   EXPECT_TRUE(errorToBool(UnknownVar.takeError()));
 
   // Clear local variables and check they become absent.
   Cxt.clearLocalVars();
   LocalVar = Cxt.getPatternVarValue(LocalVarStr);
   EXPECT_TRUE(errorToBool(LocalVar.takeError()));
   // Check a numeric expression's evaluation fails if called after clearing of
   // local variables, if it was created before. This is important because local
   // variable clearing due to --enable-var-scope happens after numeric
   // expressions are linked to the numeric variables they use.
-  EXPECT_TRUE(errorToBool((*Expression)->eval().takeError()));
+  EXPECT_TRUE(errorToBool((*ExpressionAST)->eval().takeError()));
   P = FileCheckPattern(Check::CheckPlain, &Cxt, 2);
-  Expression = P.parseNumericSubstitutionBlock(LocalNumVarRef,
-                                               DefinedNumericVariable, SM);
-  EXPECT_TRUE(bool(Expression));
-  ExpressionVal = (*Expression)->eval();
+  ExpressionAST = P.parseNumericSubstitutionBlock(
+      LocalNumVarRef, DefinedNumericVariable, /*IsLegacyLineExpr=*/false, SM);
+  EXPECT_TRUE(bool(ExpressionAST));
+  ExpressionVal = (*ExpressionAST)->eval();
   EXPECT_TRUE(errorToBool(ExpressionVal.takeError()));
   EmptyVar = Cxt.getPatternVarValue(EmptyVarStr);
   EXPECT_TRUE(errorToBool(EmptyVar.takeError()));
   // Clear again because parseNumericSubstitutionBlock would have created a
   // dummy variable and stored it in GlobalNumericVariableTable.
   Cxt.clearLocalVars();
 
   // Redefine global variables and check variables are defined again.
   GlobalDefines.emplace_back(std::string("$GlobalVar=BAR"));
   GlobalDefines.emplace_back(std::string("#$GlobalNumVar=36"));
   EXPECT_FALSE(errorToBool(Cxt.defineCmdlineVariables(GlobalDefines, SM)));
   StringRef GlobalVarStr = "$GlobalVar";
   StringRef GlobalNumVarRef = bufferize(SM, "$GlobalNumVar");
   Expected<StringRef> GlobalVar = Cxt.getPatternVarValue(GlobalVarStr);
   EXPECT_TRUE(bool(GlobalVar));
   EXPECT_EQ(*GlobalVar, "BAR");
   P = FileCheckPattern(Check::CheckPlain, &Cxt, 3);
-  Expression = P.parseNumericSubstitutionBlock(GlobalNumVarRef,
-                                               DefinedNumericVariable, SM);
-  EXPECT_TRUE(bool(Expression));
-  ExpressionVal = (*Expression)->eval();
+  ExpressionAST = P.parseNumericSubstitutionBlock(
+      GlobalNumVarRef, DefinedNumericVariable, /*IsLegacyLineExpr=*/false, SM);
+  EXPECT_TRUE(bool(ExpressionAST));
+  ExpressionVal = (*ExpressionAST)->eval();
   EXPECT_TRUE(bool(ExpressionVal));
   EXPECT_EQ(*ExpressionVal, 36U);
 
   // Clear local variables and check global variables remain defined.
   Cxt.clearLocalVars();
   EXPECT_FALSE(errorToBool(Cxt.getPatternVarValue(GlobalVarStr).takeError()));
   P = FileCheckPattern(Check::CheckPlain, &Cxt, 4);
-  Expression = P.parseNumericSubstitutionBlock(GlobalNumVarRef,
-                                               DefinedNumericVariable, SM);
-  EXPECT_TRUE(bool(Expression));
-  ExpressionVal = (*Expression)->eval();
+  ExpressionAST = P.parseNumericSubstitutionBlock(
+      GlobalNumVarRef, DefinedNumericVariable, /*IsLegacyLineExpr=*/false, SM);
+  EXPECT_TRUE(bool(ExpressionAST));
+  ExpressionVal = (*ExpressionAST)->eval();
   EXPECT_TRUE(bool(ExpressionVal));
   EXPECT_EQ(*ExpressionVal, 36U);
 }
 } // namespace