Page MenuHomePhabricator

FileCheck [5/12]: Introduce regular numeric variables
ClosedPublic

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

Details

Summary

This patch is part of a patch series to add support for FileCheck
numeric expressions. This specific patch introduces regular numeric
variables which can be set on the command-line.

This commit introduces regular numeric variable that can be set on the
command-line with the -D option to a numeric value. They can then be
used in CHECK patterns in numeric expression with the same shape as
@LINE numeric expression, ie. VAR, VAR+offset or VAR-offset where offset
is an integer literal.

The commit also enable strict whitespace in the verbose.txt testcase to
check that the position or the location diagnostics. It fixes one of the
existing CHECK in the process which was not accurately testing a
location diagnostic (ie. the diagnostic was correct, not the CHECK).

Copyright:

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

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
probinson added inline comments.May 9 2019, 7:16 AM
llvm/lib/Support/FileCheck.cpp
1588 ↗(On Diff #198684)

collisions ... variables

1628 ↗(On Diff #198684)

collisions ... variables

llvm/test/FileCheck/numeric-defines-diagnostics.txt
30 ↗(On Diff #198684)

Maybe also -D#VALUE3=VALUE1+2 (i.e. without quotes or whitespace).

jhenderson added inline comments.May 9 2019, 7:29 AM
llvm/test/FileCheck/pattern-defines-diagnostics.txt
15 ↗(On Diff #198684)

Nit: Missin -> missing (sorry, didn't see this before)

llvm/test/FileCheck/var-scope.txt
4–6 ↗(On Diff #198684)

What's the purpose of having these 3 as separate RUN lines, rather than a single one with check-prefixes=CHECK,GLOBAL,LOCAL3?

llvm/test/FileCheck/verbose.txt
135 ↗(On Diff #198684)

Why has this changed?

llvm/unittests/Support/FileCheckTest.cpp
23 ↗(On Diff #198684)

What's the purpose of the cast here and elsewhere in this test?

42 ↗(On Diff #198684)

Why is this a pointer here?

43 ↗(On Diff #198684)

Why is this a pointer?

147 ↗(On Diff #198684)

Why struct?

229 ↗(On Diff #198684)

Why is this a pointer?

269 ↗(On Diff #198684)

Why is this a pointer?

295 ↗(On Diff #198684)

"Empty variable name"?

303 ↗(On Diff #198684)

"Invalid variable name"?

352 ↗(On Diff #198684)

Do you need to be explicit about the unsigned-ness here?

361–366 ↗(On Diff #198684)

I'm not sure I follow the purpose of this test case addition.

386 ↗(On Diff #198684)

Is it necessary to be explicit about the unsigned-ness here?

398 ↗(On Diff #198684)

Is it necessary to be explicit about the unsigned-ness here?

thopre updated this revision to Diff 198866.May 9 2019, 10:08 AM
thopre marked 32 inline comments as done.

Address all outstanding comments

thopre added inline comments.May 9 2019, 10:09 AM
llvm/lib/Support/FileCheck.cpp
27–51 ↗(On Diff #194480)

As Paul mentioned, I've addressed your other comment regarding using llvm::Optional instead of having an uint64_t and a bool. I've also adapted functions accordingly and some of them became trivial. This one was not much change since I need to call EvalBinop when the value is defined so I still need an if and separate return depending on whether the Optional value is true or not. Marking the issue as done since it seems I addressed the issue you were raising.

llvm/test/FileCheck/var-scope.txt
4–6 ↗(On Diff #198684)

None, inherited from the original regex-scope.txt file. I've combined them now.

llvm/test/FileCheck/verbose.txt
135 ↗(On Diff #198684)

As per the patch description, I've enabled --strict-whitespace for these tests to check that the location information is correct in the CHECK I was adding but this check was failing. I've checked that the number of space I'm changing it to matches the column information from the diagnostic (18 spaces, caret on the 19th column).

llvm/unittests/Support/FileCheckTest.cpp
23 ↗(On Diff #198684)

To avoid a warning about comparison between sined (42) and unsigned (*Value which is uint64_t) integers. Elsewhere I've suffixed the literal with U, I've now normalized and that approach (ie. 42U in this case)

42 ↗(On Diff #198684)

My bad, must have been a copy/paste from somewhere else. FileCheckNumericVariable are referenced by a std::unique_ptr in one of the member of FileCheckPatternContext so need to be a pointer to avoid freeing something from the stack in some of the unit test.

43 ↗(On Diff #198684)

Likewise, bad copy/paste. Fixed.

352 ↗(On Diff #198684)

Yes to avoid a compiler warning, see earlier comment.

361–366 ↗(On Diff #198684)

Numeric expressions are linked to numeric variable structure at parse time but clearing of local variable happens at runtime. It is therefore important that evaluating a cleared variable gives an error even if we have a pointer from before it was cleared. I've updated the comment to explain it.

386 ↗(On Diff #198684)

Yes, see earlier comment.

398 ↗(On Diff #198684)

Ditto.

thopre updated this revision to Diff 198946.May 9 2019, 4:49 PM

Fix codestyle

I may have missed it, but I'm not sure I saw any unit tests for parsing a numeric expression involve an addition or subtraction?

llvm/test/FileCheck/var-scope.txt
1 ↗(On Diff #198946)

This test has the same mixed -/-- as mentioned in other test cases. Please fix!

llvm/unittests/Support/FileCheckTest.cpp
18 ↗(On Diff #198946)

Maybe worth adding a check of getName here.

24 ↗(On Diff #198946)

Check getValue() here after setting, to show that the new value is returned for a defined variable.

41 ↗(On Diff #198946)

Undef? Maybe this test should be just called NumExpr, since it tests the entire class.

45 ↗(On Diff #198946)

undef -> undefined here and below

58 ↗(On Diff #198946)

I might be worth showing somewhere that getUndefVarName can be called without an eval call.

183 ↗(On Diff #198946)

Nit: full stop here and next comment.

228–229 ↗(On Diff #198946)

I've lost track a little. Is @LINE the only allowed numeric variable currently, or can we have others now (I thought we could)? If we can, we should have a non-pseudo variable test case here (possibly in addition to a pseudo one).

255 ↗(On Diff #198946)

Explicitly say that "getUndefVarName returns ..." instead of "Undef var ..." here and below.

275 ↗(On Diff #198946)

What is this comment line referring to?

361–363 ↗(On Diff #198946)

variable -> variables

Based on your comment, I think this can be further improved:

"Check a numeric expression's eval fails if called after clearing of local variables, if it was created before. This is important because local variable clearing because of --enable-var-scope happens after numeric expressions are linked to the numeric variables they use."

23 ↗(On Diff #198684)

Okay. I feel like it's a bug if that warning comes out and the signed number is known to be a positive literal, but that's a separate issue.

thopre updated this revision to Diff 199131.May 11 2019, 12:07 AM
thopre marked 14 inline comments as done.

Address outstanding comments

thopre added inline comments.May 11 2019, 12:32 AM
llvm/unittests/Support/FileCheckTest.cpp
24 ↗(On Diff #198946)

I suppose you mean the old value. As mentioned in the comment, setValue is supposed to fail when the variable is already defined and the value is then unchanged.

23 ↗(On Diff #198684)

Yes but not a bug in FileCheck :-)

jhenderson added inline comments.May 13 2019, 3:26 AM
llvm/unittests/Support/FileCheckTest.cpp
239 ↗(On Diff #199131)

Shouldn't this use something other than "@LINE"? How does this even work given its used in a substitution "N"?

282 ↗(On Diff #199131)

is empty -> returns an empty string.

288 ↗(On Diff #199131)

does not return anything (empty StringRef) -> returns an empty string.

thopre updated this revision to Diff 199234.May 13 2019, 3:43 AM
thopre marked 4 inline comments as done.

Address outstanding review comments

llvm/unittests/Support/FileCheckTest.cpp
239 ↗(On Diff #199131)

The substitution only cares about what variable object it points to. The name in the variable object is used only to populate the name of undefined variables in a substitution. Anyway, fixed now.

jhenderson accepted this revision.May 13 2019, 4:23 AM

Okay, LGTM, but make sure @probinson is happy at least.

This revision is now accepted and ready to land.May 13 2019, 4:23 AM

Okay, LGTM, but make sure @probinson is happy at least.

He wrote "I am pretty happy with this now, maybe give the other people who have been participating a day or so to chime in again" earlier so will commit this now. Thanks everyone who has tirelessly reviewed this patch! It's good to get the first big patch adding support for numeric expression landing.

This revision was automatically updated to reflect the committed changes.

Okay, LGTM, but make sure @probinson is happy at least.

He wrote "I am pretty happy with this now, maybe give the other people who have been participating a day or so to chime in again" earlier so will commit this now. Thanks everyone who has tirelessly reviewed this patch! It's good to get the first big patch adding support for numeric expression landing.

FYI I've noticed the sphinx error in [1] and am investigated it. I changed laptop recently and sphinx build was not enabled on the new one which is why I missed it. Am doing a build now with sphinx enabled and will send a patch as soon as I can reproduce the issue.

[1] http://lab.llvm.org:8011/builders/llvm-sphinx-docs/builds/31133/steps/docs-llvm-html/logs/stdio

Okay, LGTM, but make sure @probinson is happy at least.

He wrote "I am pretty happy with this now, maybe give the other people who have been participating a day or so to chime in again" earlier so will commit this now. Thanks everyone who has tirelessly reviewed this patch! It's good to get the first big patch adding support for numeric expression landing.

FYI I've noticed the sphinx error in [1] and am investigated it. I changed laptop recently and sphinx build was not enabled on the new one which is why I missed it. Am doing a build now with sphinx enabled and will send a patch as soon as I can reproduce the issue.

[1] http://lab.llvm.org:8011/builders/llvm-sphinx-docs/builds/31133/steps/docs-llvm-html/logs/stdio

I'm afraid I cannot reproduce this error. I'm using sphinx-build 1.8.5 from the python-sphinx Ubuntu package version 1.6.7-1ubuntu1. Can anyone with a different version try to reproduce? Thanks in advance.

jhenderson added a comment.EditedMay 14 2019, 1:23 AM

Okay, LGTM, but make sure @probinson is happy at least.

He wrote "I am pretty happy with this now, maybe give the other people who have been participating a day or so to chime in again" earlier so will commit this now. Thanks everyone who has tirelessly reviewed this patch! It's good to get the first big patch adding support for numeric expression landing.

FYI I've noticed the sphinx error in [1] and am investigated it. I changed laptop recently and sphinx build was not enabled on the new one which is why I missed it. Am doing a build now with sphinx enabled and will send a patch as soon as I can reproduce the issue.

[1] http://lab.llvm.org:8011/builders/llvm-sphinx-docs/builds/31133/steps/docs-llvm-html/logs/stdio

I'm afraid I cannot reproduce this error. I'm using sphinx-build 1.8.5 from the python-sphinx Ubuntu package version 1.6.7-1ubuntu1. Can anyone with a different version try to reproduce? Thanks in advance.

I don't have Sphinx setup on my machine. I'd suggest a couple of things: 1) make sure that the problem was directly caused by your change according to the build bots, and isn't a case of flakiness. 2) Ask for help on the llvm-dev mailing list, CC'ing the build bot maintainer who may be able to better reproduce. You should (unfortunately) probably also revert if you haven't so that the build bots aren't bad for everyone else's changes.

thopre reopened this revision.May 14 2019, 2:30 AM

Reopen to easily review changes to fix FileCheck.rst build failure

This revision is now accepted and ready to land.May 14 2019, 2:30 AM
thopre updated this revision to Diff 199383.May 14 2019, 2:30 AM

Fix list in FileCheck.rst

thopre updated this revision to Diff 199388.May 14 2019, 3:07 AM

Really submit fix for list in FileCheck.rst

thopre requested review of this revision.May 14 2019, 3:08 AM
thopre updated this revision to Diff 199389.May 14 2019, 3:21 AM
  • Address Paul's review comments
  • Rename numeric expression subsitution for numeric substitution to clearly distinguish numeric expression (the part that gets evaluated) from the whole #<something> that gets substituted (which might include a numeric variable definition).
  • Rename ParsePattern into parsePattern since it is modified
thopre updated this revision to Diff 199391.May 14 2019, 3:33 AM

Undo latest submission

Unfortunately, the /new link for me isn't working for this. Could you point out what has changed since your previous version?

Unfortunately, the /new link for me isn't working for this. Could you point out what has changed since your previous version?

Of course: the list introduced by "The syntax to check a numeric expression constraint is" in FileCheck.rst has now one space between the asterisks and the beginning of the items. When an item span several lines (as is the case for the second and third items in the list), the follow-on lines start by 2 spaces as required by sphinx which also allow them to nicely align with the first line. This allows sphinx to build without warning.

This revision is now accepted and ready to land.May 14 2019, 4:42 AM
This revision was automatically updated to reflect the committed changes.
rnk removed a reviewer: rnk.May 14 2019, 11:29 AM
rnk removed a subscriber: rnk.
ychen added a subscriber: ychen.Jul 1 2019, 11:41 PM