Lower CppOverloadSetValue

[ivanaivanovska]

Following up on comments from #5891 ( 1, 2).

Lowering CppOverloadSetValue as an empty struct value, using context.GetLiteralAsValue(). Also changed its constant kind to InstConstantKind::Always.

Part of #5915

[jonmeow]

This PR says it's adding lowering, but doesn't have any changes to lowering tests. Can you please add tests that demonstrate why this is being lowered?

[ivanaivanovska]

This PR says it's adding lowering, but doesn't have any changes to lowering tests. Can you please add tests that demonstrate why this is being lowered?

I added the test toolchain/lower/testdata/interop/cpp/overloads.carbon, with the the example from the comment, however I don't see this lowered. When I also tried to build and run this example in a demo, that also worked as expected:

// main.carbon
library "Main";
import Core library "io";

import Cpp inline '''
int foo() {
  return 0;
}
int foo(int a) {
  return a + 1;
}
''';

fn Run() {
  var n: i32 = 1;
  n = Cpp.foo(n);
  Core.Print(n);
} 

$ bazel-bin/toolchain/carbon compile main.carbon
$ bazel-bin/toolchain/carbon link main.o \--output=demo_carbon
$ ./demo_carbon
2 

While I'll look into why this is not lowered (maybe filling in HandleInst?), @zygoloid could you please confirm that this should be lowered in this example? Wouldn't the issue be visible somewhere when running it, if something is not lowered properly?

[jonmeow]

I added the test toolchain/lower/testdata/interop/cpp/overloads.carbon, with the the example from the comment, however I don't see this lowered.

Looking at that comment, I see:

import Cpp inline "void foo(); void foo(int);";
fn F() {
  Cpp.foo;
}

It looks like you added a function call to that example, but I think that actually drifts away from what zygoloid was suggesting to test. In the above example, Cpp.foo is the actual value of the statement.

You might want to look at toolchain/lower/testdata/primitives/type_values.carbon for an example test that may also be helpful, if zygoloid's example doesn't work -- in particular putting the value on a return boundary.

[jonmeow]

Suggested change

	// lowered as an empty struct value.
	// Lowered as an empty struct value.

[ivanaivanovska]

Done.

[ivanaivanovska]

It looks like you added a function call to that example, but I think that actually drifts away from what zygoloid was suggesting to test. In the above example, Cpp.foo is the actual value of the statement.

I changed the test to use the value Cpp.foo, but I still see no difference. You can see the output in the current version of toolchain/lower/testdata/interop/cpp/overloads.carbon.

You might want to look at toolchain/lower/testdata/primitives/type_values.carbon for an example test that may also be helpful, if zygoloid's example doesn't work -- in particular putting the value on a return boundary.

I also tried the following, but without any success:

library "[[@TEST_NAME]]";

import Cpp inline "void foo(); void foo(int);";

fn F() {
  // CHECK:STDERR: import_overload_set.carbon:[[@LINE+4]]:11: error: semantics TODO: `HandleAutoTypeLiteral` [SemanticsTodo]
  // CHECK:STDERR:   var x : auto = Cpp.foo;
  // CHECK:STDERR:           ^~~~
  // CHECK:STDERR:
  var x : auto = Cpp.foo;
  x();
}

As well as:

// --- import_overload_set.carbon

library "[[@TEST_NAME]]";

import Cpp inline "void foo(); void foo(int);";

fn F() -> type {
  // CHECK:STDERR: import_overload_set.carbon:[[@LINE+7]]:3: error: cannot implicitly convert non-type value of type `<type of Cpp.foo>` to `type` [ConversionFailureNonTypeToFacet]
  // CHECK:STDERR:   return Cpp.foo;
  // CHECK:STDERR:   ^~~~~~~~~~~~~~~
  // CHECK:STDERR: import_overload_set.carbon:[[@LINE+4]]:3: note: type `<type of Cpp.foo>` does not implement interface `Core.ImplicitAs(type)` [MissingImplInMemberAccessNote]
  // CHECK:STDERR:   return Cpp.foo;
  // CHECK:STDERR:   ^~~~~~~~~~~~~~~
  // CHECK:STDERR:
  return Cpp.foo;
}

The code of EmitAsConstant() seems to be executed, maybe it is cleaned up afterwards?

If you have any further suggestions please let me know. Thanks.

[jonmeow]

Here's a test that uses a generic to expose a value:

import Cpp inline '''
  void foo();
  void foo(int);
''';

fn EchoValue[ValueT:! Core.Copy](value:! ValueT) -> ValueT {
  return value;
}

fn CppOverloadSet() {
  EchoValue(Cpp.foo);
}

If you replace Cpp.foo with true, then you'll see something like:

// CHECK:STDOUT: define linkonce_odr i1 @_CEchoValue.Main.13326305c9417809() !dbg !18 {
// CHECK:STDOUT: entry:
// CHECK:STDOUT:   ret i1 true, !dbg !19
// CHECK:STDOUT: }

So that's demonstrating that, if Cpp.foo can be treated as a value, it'll show up in the LLVM IR.

However, this crashes right now because you're not handling the new instructions in TypeIterator. Also, this approach assumes you implementing copying of the value (Core.Copy), which hopefully is reasonable for an overload? I think it'd be harder to test if it remains non-copyable.

[ivanaivanovska]

So that's demonstrating that, if Cpp.foo can be treated as a value, it'll show up in the LLVM IR.

However, this crashes right now because you're not handling the new instructions in TypeIterator. Also, this approach assumes you implementing copying of the value (Core.Copy), which hopefully is reasonable for an overload? I think it'd be harder to test if it remains non-copyable.

I see. Handling the new instructions in TypeIterator is not an issue, but I’m not sure about copying. Syncing with @zygoloid offline, the conclusion was that the lowering is not reachable at the moment, but landing the PR would still be beneficial. I see that we have a better structured SemIR now and we have the CppOverloadSetValue set-up to be lowered when we can explore such use cases. Would that make sense to you?

[jonmeow]

Sure, it seems fine to not test lowering if zygoloid thinks it can't be lowered. Do you plan to fix the crashing part of this PR, though? e.g., this can be tested in check:

// --- fail_missing_impl.carbon
library "[[@TEST_NAME]]";

import Cpp inline '''
  void foo();
  void foo(int);
''';

interface I {}

fn EchoValue[ValueT:! I](value:! ValueT) {}

fn CppOverloadSet() {
  EchoValue(Cpp.foo);
}

[ivanaivanovska]

Sure, it seems fine to not test lowering if zygoloid thinks it can't be lowered. Do you plan to fix the crashing part of this PR, though? e.g., this can be tested in check:

// --- fail_missing_impl.carbon
library "[[@TEST_NAME]]";

import Cpp inline '''
  void foo();
  void foo(int);
''';

interface I {}

fn EchoValue[ValueT:! I](value:! ValueT) {}

fn CppOverloadSet() {
  EchoValue(Cpp.foo);
}

Sounds good. Added the test in toolchain/check/testdata/interop/cpp/function/overloads.carbon. PTAL.

[jonmeow]

LG