Add support for operators on `Core.IntLiteral`. #4716

zygoloid · 2024-12-19T20:06:52Z

Fixes integer builtins to produce the correct values (and not CHECK-fail) when used on integer literals. Also adds impls to the prelude to use the new builtins to perform operations on integer literals.

Perhaps most importantly, this allows directly initializing i32 values with negative numbers, as the negation operation on integer literals now works.

For testing I've added tests for use of literals with one operator in each class (addition, multiplication, ordering, bitwise, etc) for which there are distinct rules or overflow behavior, rather than exhaustively testing all the combinations. This is aimed at finding a good tradeoff between maintainability of the tests and thorough test coverage.

Also fixes lowering of heterogeneous shifts and comparisons. These are currently disabled when one of the operands is an integer literal, but we may want to allow that when the integer literal operand has a known constant value.

Also add support for mixed comparison between different integer types.

Stop trying to allow operations on IntLiterals to be lowered. That's not possible in general because we don't necessarily have a value at runtime for the IntLiteral.

toolchain/check/eval.cpp

geoffromer · 2024-12-20T00:02:29Z

toolchain/check/eval.cpp

@@ -769,6 +766,15 @@ static auto DiagnoseDivisionByZero(Context& context, SemIRLoc loc) -> void {
  context.emitter().Emit(loc, CompileTimeDivisionByZero);
 }

+// Get an integer at a suitable bit-width: either its actual width if it has a
+// fixed width, or the canonical width from the value store if not.
+static auto GetIntAtSuitableWidth(Context& context, IntId bit_width_id,


Might it make sense to make this behavior part of GetAtWidth, rather than a separate function?

Hm. I think the need to deal with the "unknown width" case in exactly this way is probably specific to the evaluation logic, and other users of IntStore are unlikely to want it -- especially in something with such an innocuous name :) It looks like all the current calls to GetAtWidth are in this file, but I think that's just because lower/constant.cpp hasn't been updated to use it yet -- this is precisely GetAtWidth written longhand, and doesn't want this special casing for an invalid width.

geoffromer · 2024-12-20T00:04:07Z

toolchain/check/eval.cpp

+static auto GetIntAtSuitableWidth(Context& context, IntId bit_width_id,
+                                  IntId int_id) -> llvm::APInt {


It seems potentially pretty confusing that this function takes its parameters in the opposite order from GetAtWidth.

Yeah, especially given the parameters are all the same types. I'm a little bit uneasy with having the bit width last given the signatures of GetIntAtSuitableWidth / GetIntsAtSuitableWidth -- having the "variadic" part last feels better to me -- but matching GetAtWidth and making the parameter order match the order in which the things are mentioned in the name is probably reasonable. Done.

geoffromer · 2024-12-20T19:01:34Z

toolchain/check/eval.cpp

+    }
+
+    default:
+      // Break to do additional setup for other builtin kinds.


I don't quite follow: if we break here, we don't just do "additional setup", we actually perform the operations, right?

(Pre-existing.) Yeah, this seems confusing -- and in fact the shift logic is completely different to the rest of this, so I've split that out into a different function.

geoffromer · 2024-12-20T19:15:03Z

toolchain/check/eval.cpp

+  if (result.overflow && !lhs_bit_width_id.is_valid()) {
+    // Retry with a larger bit width. Most operations can only overflow by one
+    // bit, but signed n-bit multiplication can overflow to 2n-1 bits.
+    int new_width =


Why not do this before the first attempt, and avoid the need to retry? If the concern is that 2n bits could be too expensive if it's not needed, it seems like we could compute a tighter upper bound pretty efficiently (something like lhs_val.ceilLogBase2() + rhs_val.ceilLogBase2()?)

I originally planned to do that, but @chandlerc suggested it'd be better to do it this way -- IIUC the rationale is that we'll almost never need to go to a wider size than the inputs (because they've already been rounded up to a multiple of 64 bits by the IntStore), so it's better to speculatively assume that the result will fit than to spend time computing a width -- especially because any wider upper bound will require a heap allocation (APInt heap allocates integers wider than 64 bits) and wider operations get more expensive pretty quickly in APInt at least.

Yep.

But it seems good to capture this rationale in the comments as otherwise it is a bit mysterious why we wait to see the overflow before doing this.

geoffromer · 2024-12-20T19:52:06Z

toolchain/check/testdata/builtins/int/left_shift.carbon

+var a_lit: Core.IntLiteral() = 12;
+var an_i32: i32 = 34;
+
+// This can't be valid: we don't have a compile-time or runtime integer value for `n`.


What does "n" refer to in this context?

Oops, missed this when I renamed the variable. Fixed.

geoffromer · 2024-12-20T20:01:01Z

toolchain/check/testdata/builtins/int/less_eq.carbon

Is less_eq meaningfully different from greater_eq?

No, I'd just already updated both before I decided it wasn't worth it. I can revert the changes to one of them if you like.

geoffromer · 2024-12-20T20:11:04Z

toolchain/check/testdata/builtins/int/sdiv.carbon

I assume the thinking here is that while sdiv has a special rule about 0, that rule is sufficiently orthogonal to fixed-vs-variable width that we don't need separate coverage of non-fixed-width cases?

In cases like this where we're relying on tests on another operation to provide coverage indirectly, I wonder if it might make sense to have a comment explaining that, and pointing to the tests that are believed to provide that coverage.

Hm. Thinking about this again, I think it probably is worth testing the weird case where sdiv can overflow -- but not for IntLiteral. And while testing that I think it also makes sense to explicitly test division by zero, which is another overflow-like case but one that can happen for IntLiteral. Done.

geoffromer · 2024-12-20T20:12:52Z

toolchain/check/testdata/builtins/int/smul.carbon

Might it make sense to have a test of non-fixed-width overflow, or would that be too slow?

It's just too slow. I tried forming a value somewhat near our bit limit using a left-shift (which I think is the fastest way we have to do that) and it ran for a very long time just doing the shift. I think the APInt multiply algorithm is probably quadratic in the length of the int, too... (I can't imagine it's doing a Fourier transform to speed it up!)

geoffromer · 2024-12-20T20:29:27Z

toolchain/lower/testdata/builtins/int.carbon

I don't know LLVM IR yet, so it could take me a while to review the changes in this directory, especially since IIUC the golden outputs are supposed to be reviewed more carefully in lower than in check. Feel free to find another reviewer for this part if you want to expedite things.

I've asked @chandlerc to take a look.

chandlerc

The LLVM IR looks pretty good. Left some comments throughout.

A higher level meta comment though: I think we need a way to suppress the SemIR from test splits where we're able to fully validate the behavior with the type system as you're doing with Expect(<some value>), or where we're just testing diagnostics. The SemIR created by these file splits is huge and completely unhelpful given that they are self enforcing.

Not sure that's strictly necessary prior to landing this PR, but it was already hard to just navigate the SemIR added by this PR.

And if anything, we should be leveraging all the opportunities we have to directly test things the way you are and bypass the more complex SemIR-based testing and only do that in a few places where we really want to zoom into how this is represented, not how it behaves.

chandlerc · 2024-12-21T02:01:06Z

toolchain/check/eval.cpp

+  if (result.overflow && !lhs_bit_width_id.is_valid()) {
+    // Retry with a larger bit width. Most operations can only overflow by one
+    // bit, but signed n-bit multiplication can overflow to 2n-1 bits.
+    int new_width =


Yep.

But it seems good to capture this rationale in the comments as otherwise it is a bit mysterious why we wait to see the overflow before doing this.

toolchain/check/eval.cpp

toolchain/check/testdata/builtins/int/and.carbon

toolchain/lower/handle_call.cpp

toolchain/sem_ir/builtin_function_kind.cpp

danakj · 2024-12-23T18:11:37Z

toolchain/check/eval.cpp

-  // the RHS with the LHS bit width.
-  CARBON_CHECK(rhs.type_id == lhs.type_id, "Heterogeneous builtin integer op!");
-  llvm::APInt rhs_val = context.ints().GetAtWidth(rhs.int_id, lhs_bit_width_id);
+  return {.lhs = context.ints().GetAtWidth(lhs_id, bit_width_id),


The comment on APIntBinaryOperands talks about RVO, but doesn't this function defeat that by returning a named variable in one path and a temporary in another?

This return statement will have RVO / copy elision applied, regardless of whatever else the function does, because it's directly constructing an instance of the return type in the returned expression.

The other return statement will typically have NRVO applied, because all the returns in the scope of result return result (though NRVO is not guaranteed by the language rules).

danakj · 2024-12-23T18:23:09Z

toolchain/check/eval.cpp

+    // Retry with a larger bit width. Most operations can only overflow by one
+    // bit, but signed n-bit multiplication can overflow to 2n-1 bits.
+    int new_width =
+        builtin_kind == SemIR::BuiltinFunctionKind::IntSMul


Does this not seem to imply that IntUMul maybe have introduced wrapping on the first ComputeBinaryIntOpResult() attempt, and wouldn't if we'd increased its bitwidth? Is that desirable?

Unsigned operations aren't meaningful on an unsized integer type, so that's not even possible here. I've extended the comment to explain and added a CHECK.

Add `EXTRA-ARGS:` support to file_test, to add arguments without overriding the default arguments. Use `EXTRA-ARGS: --no-dump-sem-ir` to turn off SemIR dumping and thus SemIR testing in the int builtin tests, which validate correct behavior through diagnostics instead. This doesn't get us any closer to supporting more targeted SemIR dumping / testing, but hopefully this is a generally useful feature for argument testing. Requested in review of carbon-language#4716.

Add `EXTRA-ARGS:` support to file_test, to add arguments without overriding the default arguments. Use `EXTRA-ARGS: --no-dump-sem-ir` to turn off SemIR dumping and thus SemIR testing in the int builtin tests, which validate correct behavior through diagnostics instead. This doesn't get us any closer to supporting more targeted SemIR dumping / testing, but this seems to be a generally useful feature anyway. Most existing tests using `ARGS` have been switched over to using `EXTRA-ARGS`. Requested in review of #4716.

chandlerc

(I think this is mostly waiting to get rebased on the test improvement so the churn there is removed, but I should also go back through other comments I suspect)

toolchain/sem_ir/builtin_function_kind.cpp

builtins.

chandlerc

Some diagnostic improvements for the future I've flagged below, but those shouldn't be blocking. I think this is already at the point of a pretty huge improvement over the status quo and so motivated to land it and start iterating.

There may be some more API improvements possible, especially around eval.cpp, but again, I think those can reasonably be done as follow-ups if needed.

LGTM

chandlerc · 2024-12-31T02:10:53Z

toolchain/check/testdata/tuple/access/fail_negative_indexing.carbon

@@ -9,9 +9,9 @@
 // TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/tuple/access/fail_negative_indexing.carbon

 var a: (i32, i32) = (12, 6);
-// CHECK:STDERR: fail_negative_indexing.carbon:[[@LINE+3]]:17: error: cannot access member of interface `Core.Negate` in type `Core.IntLiteral` that does not implement that interface [MissingImplInMemberAccess]
+// CHECK:STDERR: fail_negative_indexing.carbon:[[@LINE+3]]:14: error: tuple element index `-10` is past the end of type `(i32, i32)` [TupleIndexOutOfBounds]


Follow-up: we should update this diagnostic to special case negative literals.

chandlerc · 2024-12-31T02:11:06Z

toolchain/check/testdata/index/fail_negative_indexing.carbon

@@ -9,7 +9,7 @@
 // TIP:   bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/index/fail_negative_indexing.carbon

 var c: [i32; 2] = (42, 42);
-// CHECK:STDERR: fail_negative_indexing.carbon:[[@LINE+3]]:16: error: cannot access member of interface `Core.Negate` in type `Core.IntLiteral` that does not implement that interface [MissingImplInMemberAccess]
+// CHECK:STDERR: fail_negative_indexing.carbon:[[@LINE+3]]:16: error: array index `-10` is past the end of type `[i32; 2]` [ArrayIndexOutOfBounds]


Follow-up: we should update this diagnostic to special case negative literals.

zygoloid added 7 commits December 18, 2024 23:09

Add support for IntLiteral operators.

bfb1a96

Also add support for mixed comparison between different integer types.

Add IntLiteral operations to the prelude.

55e5b83

Testing for new ops.

312a778

Fix heterogeneous shift lowering.

6fbd9b6

More testing.

d25730f

Stop trying to allow operations on IntLiterals to be lowered. That's not possible in general because we don't necessarily have a value at runtime for the IntLiteral.

pre-commit fixes

106a94d

Also disallow runtime comparisons against integer literals for now.

bb7606f

github-actions bot added the toolchain label Dec 19, 2024

github-actions bot requested a review from geoffromer December 19, 2024 20:07

zygoloid added 2 commits December 19, 2024 20:08

Add test for directly writing INT_MIN.

97879ab

Merge branch 'trunk' into toolchain-intliteral-ops

3b27a1f

zygoloid added a commit to zygoloid/carbon-lang that referenced this pull request Dec 19, 2024

Remove workarounds for carbon-language#4716.

8b99639

zygoloid mentioned this pull request Dec 19, 2024

Solution for advent of code day 4. #4718

Open

geoffromer reviewed Dec 20, 2024

View reviewed changes

zygoloid added 3 commits December 20, 2024 22:56

Address some review comments, split bit shift out into its own function.

8722548

Address some more review comments.

ab54ae8

Add test for lowering mixed comparisons.

26d9bc7

zygoloid requested review from chandlerc and geoffromer December 20, 2024 23:58

chandlerc reviewed Dec 21, 2024

View reviewed changes

danakj reviewed Dec 23, 2024

View reviewed changes

zygoloid added a commit to zygoloid/carbon-lang that referenced this pull request Dec 27, 2024

Remove workarounds for carbon-language#4716.

4c5629d

zygoloid added 2 commits December 30, 2024 21:37

Comments and CHECKs for review comments.

93cd66c

Merge branch 'trunk' into toolchain-intliteral-ops

b2882f7

zygoloid mentioned this pull request Dec 30, 2024

Suppress testing SemIR in int builtin tests. #4748

Merged

zygoloid requested a review from chandlerc December 30, 2024 22:32

chandlerc reviewed Dec 31, 2024

View reviewed changes

toolchain/sem_ir/builtin_function_kind.cpp Outdated Show resolved Hide resolved

Merge branch 'trunk' into toolchain-intliteral-ops

cae63b4

Add a big comment explaining what's going on for integer literal

0f947d7

builtins.

zygoloid requested a review from chandlerc December 31, 2024 02:00

chandlerc approved these changes Dec 31, 2024

View reviewed changes

Merge branch 'trunk' into toolchain-intliteral-ops

6a9275a

zygoloid enabled auto-merge December 31, 2024 06:23

zygoloid added this pull request to the merge queue Dec 31, 2024

Merged via the queue into carbon-language:trunk with commit 4a7aefe Dec 31, 2024
8 checks passed

zygoloid deleted the toolchain-intliteral-ops branch December 31, 2024 06:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for operators on `Core.IntLiteral`. #4716

Add support for operators on `Core.IntLiteral`. #4716

zygoloid commented Dec 19, 2024

geoffromer Dec 20, 2024

zygoloid Dec 20, 2024

geoffromer Dec 20, 2024

zygoloid Dec 20, 2024

geoffromer Dec 20, 2024

zygoloid Dec 20, 2024

geoffromer Dec 20, 2024

zygoloid Dec 20, 2024 •

edited

Loading

chandlerc Dec 21, 2024

geoffromer Dec 20, 2024

zygoloid Dec 20, 2024

geoffromer Dec 20, 2024

zygoloid Dec 20, 2024

geoffromer Dec 20, 2024

zygoloid Dec 20, 2024

geoffromer Dec 20, 2024

zygoloid Dec 20, 2024

geoffromer Dec 20, 2024

zygoloid Dec 21, 2024

chandlerc left a comment

chandlerc Dec 21, 2024

danakj Dec 23, 2024

zygoloid Dec 30, 2024

danakj Dec 23, 2024

zygoloid Dec 30, 2024

chandlerc left a comment

chandlerc left a comment

chandlerc Dec 31, 2024

chandlerc Dec 31, 2024

		static auto GetIntAtSuitableWidth(Context& context, IntId bit_width_id,
		IntId int_id) -> llvm::APInt {

Add support for operators on Core.IntLiteral. #4716

Add support for operators on Core.IntLiteral. #4716

Conversation

zygoloid commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zygoloid Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandlerc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandlerc left a comment

Choose a reason for hiding this comment

chandlerc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add support for operators on `Core.IntLiteral`. #4716

Add support for operators on `Core.IntLiteral`. #4716

zygoloid Dec 20, 2024 •

edited

Loading