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.

chandlerc · 2024-12-21T02:14:37Z

toolchain/check/eval.cpp

+    // Note that this can in theory still overflow if we limited `new_width` to
+    // `MaxIntWidth`. In that case we fall through to the signed overflow
+    // diagnostic below.
+    result = ComputeBinaryIntOpResult(builtin_kind, lhs_val, rhs_val);


Maybe:

Suggested change

result = ComputeBinaryIntOpResult(builtin_kind, lhs_val, rhs_val);

result = ComputeBinaryIntOpResult(builtin_kind, lhs_val, rhs_val);

CARBON_CHECK(new_width == IntStore::MaxIntWidth || !result.overflow);

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

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

All the new tests in this file result in a pretty staggering amount of SemIR being printed with unusually low value.

These tests actually check that they work correctly intrinsically or in the diagnostics being emitted, and so the SemIR is especially unfortunate here. Not sure there is anything to do in this PR but something to think about in terms of whene the SemIR isn't as helpful.

chandlerc · 2024-12-21T02:39:07Z

toolchain/lower/handle_call.cpp

+  // TODO: This might be an awkward size, such as 33 or 65 bits, for a
+  // signed/unsigned comparison. Would it be better to round this up to a
+  // "nicer" bit width?


My memory is that we experimented with this when considering these semantics for C++ and LLVM did a good job of handling the "odd" size and producing the expected result... But yeah, we can always revisit if practice proves otherwise.

chandlerc · 2024-12-21T02:54:05Z

toolchain/lower/handle_call.cpp

+  // Weirdly, LLVM requires the operands of bit shift operators to be of the
+  // same type. We can always use the width of the LHS, because if the RHS
+  // doesn't fit in that then the cast is out of range anyway.
+  //
+  // TODO: In a development build we should trap in that case.
+  rhs = context.builder().CreateZExtOrTrunc(rhs, lhs->getType(), "rhs");


Maybe worth commenting here that LLVM always treats the shift amount as an unsigned amount, hence ZExt is correct? And maybe add checking for negative shifts to the TODO?

chandlerc · 2024-12-21T03:05:02Z

toolchain/sem_ir/builtin_function_kind.cpp

+      // TODO: We could allow these in the case where the operand has a
+      // compile-time value.


What about when the integer literal value can be represented without change in the non-literal operand's type?

Or do we handle that by converting the literal?

I would expect fn F(i: i32) -> bool { return i < 32; } to work fine... If this is handled by conversion, maybe just mention that in the comment to avoid confusion?

chandlerc · 2024-12-21T03:07:04Z

toolchain/sem_ir/builtin_function_kind.cpp

+      // Shifts by an integer literal amount are compile-time only. We don't
+      // have a value for the shift amount at runtime in general.
+      // TODO: We could allow these in the case where the shift amount has a
+      // compile-time value.


Given that the shift amount has to fit into the shifted operand's type (and LLVM even makes us use that!), why not support there?

Or maybe this is what you mean by "compile-time value" -- but don't integer literals always have a compile time value?

Sorry if the comment is just confusing me (see below as well).

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?

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?

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

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

chandlerc Dec 21, 2024

chandlerc Dec 21, 2024

chandlerc Dec 21, 2024

chandlerc Dec 21, 2024

chandlerc Dec 21, 2024

chandlerc Dec 21, 2024

danakj Dec 23, 2024

danakj Dec 23, 2024

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

	result = ComputeBinaryIntOpResult(builtin_kind, lhs_val, rhs_val);
	result = ComputeBinaryIntOpResult(builtin_kind, lhs_val, rhs_val);
	CARBON_CHECK(new_width == IntStore::MaxIntWidth \|\| !result.overflow);

		// TODO: We could allow these in the case where the operand has a
		// compile-time value.

Add support for operators on Core.IntLiteral. #4716

Are you sure you want to change the base?

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

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

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

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

zygoloid Dec 20, 2024 •

edited

Loading