Page MenuHomePhabricator

[FileCheck] Implement * and / operators for ExpressionValue.
ClosedPublic

Authored by paulwalker-arm on Jun 1 2020, 3:28 AM.

Diff Detail

Event Timeline

paulwalker-arm created this revision.Jun 1 2020, 3:28 AM
jhenderson added a subscriber: jhenderson.

Thanks for the patch. Don't forget to add reviewers in the future though! I don't have time to look at this today (probably), so I'll come back to it tomorrow, I hope.

One of the suggestions for D79936 requires equality support and since it also makes the div-by-zero test nicer I've added it to this patch.

Oh, I thought when we were talking about operators, we were talking about the front end FileCheck ability to use them, like we have with addition and subtraction already! That would be useful too (particularly multiplication to allow for something like N*sizeof(struct) in the check patterns, but I'm okay waiting for it.

llvm/lib/Support/FileCheck.cpp
254

We've tended to use static_cast rather than C-style casts in this code. Doesn't this check also assume 2's complement? I'd expect to see something involving the min of int64_t explicitly. The operator- code has some example dancing around that you could adapt to achive this, I think.

286

Same comment as above - I think this assumes 2's complement, which we shouldn't be assuming.

llvm/unittests/Support/FileCheckTest.cpp
484

I think you can delete the "with negative/positive result" bit of the comments, since it's obvious this is the case.

489–492

Again, this makes the assumption of 2's complement.

513

Maybe worth testing multiplication by zero, with both ends of the range (MaxUint64/MinInt64).

520

Same as above - 2's complement assumption.

532–533

Perhaps also worth test cases showing the result of 0/0, 0/10, and 0/-10.

542

Replace these with EXPECT_TRUE/EXPECT_FALSE.

545

Also test two values that are identical except for positive/negative, e.g. 1 and -1.

551

For completeness, you probably also want to compare values that are different, e.g. 3 and 7. Same for the positive case.

Oh, I thought when we were talking about operators, we were talking about the front end FileCheck ability to use them, like we have with addition and subtraction already! That would be useful too (particularly multiplication to allow for something like N*sizeof(struct) in the check patterns, but I'm okay waiting for it.

I originally tried this via D79885 but as the operators all have the same precedence there was a worry that it might lead to confusion.

thopre added inline comments.Jun 2 2020, 2:32 AM
llvm/lib/Support/FileCheck.cpp
286

FYI: APInt already assumes 2 complement, see for example the negate method:

/// Negate this APInt in place.
void negate() {
  flipAllBits();
  ++(*this);
}

The reason I still tried to deal with other integer representation is that I didn't want to put more 2-complement representation and using only APInt/CheckedArithmetics was more complicated IIRW.

paulwalker-arm marked an inline comment as done.Jun 9 2020, 3:35 AM
paulwalker-arm added inline comments.
llvm/unittests/Support/FileCheckTest.cpp
520

I'm not sure what to do here. With the current design of ExpressionValue the overflow scenarios change depending on whether you're 1s or 2s complement. I started going down the path of making overflow reporting consistent (essentially making ExpressionValue a 65bit 1s complement number) but I'm worried this might not be what you want.

I could just not have tests for the cases where 1s complement differs from 2s complement, but that seems wrong, hence the request for guidance.

jhenderson added inline comments.Jun 9 2020, 3:58 AM
llvm/unittests/Support/FileCheckTest.cpp
520

Sorry for the confusion. I don't think we should change the implementation to enable consistent testing depending on the underlying numeric representation. It's probably best to simply not bother testing the edge cases so precisely, since we can't control it sensibly.

My suggestion would be to allow a bit of leeway. For example, you could show that MaxInt64 \ -1 is equal to -MaxInt64. That should be representable under the permitted numeric representations.

arichardson added inline comments.Jun 9 2020, 4:50 AM
llvm/lib/Support/FileCheck.cpp
254

The largest positive signed number can always be represented as an unsigned integer (even if you use some other representation).
operator- also does this using an implicit cast:
uint64_t MaxInt64 = std::numeric_limits<int64_t>::max();

paulwalker-arm marked 8 inline comments as done.Thu, Jun 11, 12:35 PM

I'm now doing the negation via ExpressionValue so the problematic overflow cases are handled by operator-.
I've added the requested tests and replaced the 2s complement specific one.
I've added the hooks required so the operators can be used by FileCheck.

paulwalker-arm marked 3 inline comments as done.Thu, Jun 11, 12:44 PM
paulwalker-arm marked 3 inline comments as done.Thu, Jun 11, 12:47 PM
paulwalker-arm added inline comments.
llvm/unittests/Support/FileCheckTest.cpp
542
545
551
jhenderson accepted this revision.Tue, Jun 16, 1:43 AM

LGTM, with one nit.

llvm/unittests/Support/FileCheckTest.cpp
1396–1397

Is there any particular reason you've moved the sub case later. It would help blame history if it was where it was before, and the new div case here.

This revision is now accepted and ready to land.Tue, Jun 16, 1:43 AM
paulwalker-arm marked an inline comment as done.Tue, Jun 16, 2:35 AM
paulwalker-arm added inline comments.
llvm/unittests/Support/FileCheckTest.cpp
1396–1397

This fixes a mistake in my call patch as in general I've tried to keep references to FileCheck's predefined functions in alphabetical order.

jhenderson added inline comments.Tue, Jun 16, 2:52 AM
llvm/unittests/Support/FileCheckTest.cpp
1396–1397

Okay, that's fine with me.

This revision was automatically updated to reflect the committed changes.