Page MenuHomePhabricator

FileCheck [10/12]: Add support for signed numeric values
Changes PlannedPublic

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

Details

Summary

This patch is part of a patch series to add support for FileCheck
numeric expressions. This specific patch adds support signed numeric
values, thus allowing negative numeric values.

As such, the patch adds a new class to represent a signed or unsigned
value and add the logic for type promotion and type conversion in
numeric expression mixing signed and unsigned values. It also adds
the %d format specifier to represent signed value.

Finally, it also adds underflow and overflow detection when performing a
binary operation.

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)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
thopre updated this revision to Diff 194319.Apr 9 2019, 7:19 AM

Use camel casing for new functions

thopre updated this revision to Diff 194485.Apr 10 2019, 4:45 AM

Make numeric value constructor explicit

tra removed a subscriber: tra.Apr 11 2019, 1:16 PM
thopre updated this revision to Diff 197122.Apr 29 2019, 8:37 AM

Rebase on latest changes

thopre updated this revision to Diff 198035.May 3 2019, 9:26 AM

Rebase on latest changes

thopre updated this revision to Diff 198102.May 3 2019, 3:04 PM

Rebase on latest unittest changes

thopre updated this revision to Diff 198446.May 7 2019, 5:34 AM

Rebase on top of latest changes

thopre updated this revision to Diff 198551.May 7 2019, 3:30 PM

Rebase on top of latest changes

thopre updated this revision to Diff 198648.May 8 2019, 6:53 AM

Rebase on top of latest changes

thopre updated this revision to Diff 198953.May 9 2019, 4:53 PM

Rebase on top of latest changes

thopre updated this revision to Diff 199141.May 11 2019, 7:15 AM

Rebase on top of latest changes

thopre updated this revision to Diff 199239.May 13 2019, 3:58 AM

Rebase on top of latest changes

thopre updated this revision to Diff 199399.May 14 2019, 4:03 AM

Rebase on top of latest changes

rnk removed a reviewer: rnk.May 14 2019, 11:28 AM
rnk removed a subscriber: rnk.
thopre updated this revision to Diff 200476.May 21 2019, 5:56 AM

rebase on top of latest changes

thopre updated this revision to Diff 200553.May 21 2019, 11:15 AM

rebase on top of latest changes

thopre updated this revision to Diff 201985.May 29 2019, 10:53 AM

Rebase on top of latest changes in patch series

thopre updated this revision to Diff 203212.Jun 5 2019, 11:21 AM

Rebase on top of latest changes in the series

thopre updated this revision to Diff 205631.Jun 19 2019, 10:16 AM

Rebase on top of latest changes in the patch series

thopre updated this revision to Diff 205683.Jun 19 2019, 3:15 PM
  • Fix a case of numeric expression -> expression
  • Fix code style
thopre updated this revision to Diff 205765.Jun 20 2019, 2:45 AM

Rebase on latest changes in patch series

arichardson added inline comments.Jul 26 2019, 2:59 AM
llvm/lib/Support/FileCheck.cpp
131

llvm/include/llvm/Support/CheckedArithmetic.h contains llvm::checkedAdd, llvm::checkedSub, llvm::checkedAddUnsigned and llvm::checkedSubUnsigned, which should make this code much simpler.

thopre updated this revision to Diff 233340.Dec 11 2019, 6:03 AM
thopre marked an inline comment as done.

Address comment and rebase

thopre planned changes to this revision.Dec 11 2019, 6:03 AM

Uploading the current status of this patch. I know there are still issues (in particular add more unittests), and I'll address them.

thopre planned changes to this revision.Jan 8 2020, 7:27 AM

Need to add unit tests

arichardson accepted this revision.Jan 12 2020, 3:10 AM

Looks good to me once the tests have been added and other reviewers are also happy.

llvm/lib/Support/FileCheck.cpp
208

This is fine for the initial version of the patch, but I think in the future it will be useful to permit adding an unsigned variable to a signed one, etc. as long as the result is still within the representable range.

252

I'm not sure if this is necessary but I feel like it may be useful to permit adding a signed (negative value) to an unsigned value as long as the result is >= 0. Maybe these checks should move into operator+/-?

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

Since there are quite a lot of CHECK lines now, it might be helpful to add a comment after every line that checks a new variable. For example:

11 // VAR1
12
10
-30 // VAR2
-29
-31
c // VAR3 (hex)
...

Sorry for the delay in looking at this. Comments inline.

llvm/lib/Support/FileCheck.cpp
27

I'd not abbreviate this name. It's not clear to me that "Conv" == "Conversion"

Also, it's more idiomatic to declare the Error within the if, to reduce its scope.

138

other -> Other

139

I think you need a check that Signed == other.Signed, as otherwise there will be some cases where the two values match but aren't of the same signed-ness (e.g. -1 and uint64_t max). Unless that's intentional of course.

145

I'd make the names of this and convertUnsigned a bit more explicit, by adding "to", e.g. "toSigned" or "convertToSigned"

1029

As far as I can see, this block is just skipping the error? Why the switch/case?

llvm/lib/Support/FileCheckImpl.h
133 ↗(On Diff #236825)

Perhaps a not_supported or invalid_argument would make more sense here.

158–162 ↗(On Diff #236825)

This pair of constructors feels like a recipe for confusion or compilation problems due to ambiguous resolution. I think it might be better to make Signed an additional parameter.

166–167 ↗(On Diff #236825)

other -> Other

225–229 ↗(On Diff #236825)

As with above, this could lead to ambiguous compiler resolution, if Val is neither a uint64_t or int64_t. Perhaps not an issue in internal code, but it feels like there might be a better solution to me, perhaps using templates and std::is_signed in this case?

291 ↗(On Diff #236825)

You've lost the reference to when this can be None. When can it be?

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

Maybe there should be som testing for signed positive values here?