ForwardingUtils.swift fixes and unit tests

[atrick]

No description provided.

[nate-chandler]

Would it work to add a computed property of type FunctionPassContext to TestContext?

[atrick]

Would it work to add a computed property of type FunctionPassContext to TestContext?

The key thing is that the context argument to the test function must be an instance of Context to be consistent with the APIs that are being tested. TestContext was not an instance of Context, so was pretty actively misleading.

Once you decide the test function should get a Context, you might as well give it a FunctionPassContext, which is the only one that makes sense for a FunctionTest.

[atrick]

@swift-ci test

[nate-chandler]

lgtm!

[nate-chandler]

The Windows failure is fixed by adding // REQUIRES: swift_in_compiler to forwarding_utils.sil.

[eeckstein]

Once you decide the test function should get a Context, you might as well give it a FunctionPassContext, which is the only one that makes sense for a FunctionTest.

I don't agree with that. FunctionPassContext has many APIs which are not relevant for a test and should not be called from a test.
It's better to have a separate TestContext which conforms to Context or MutatingContext and only provides the APIs which are needed for testing. To avoid code duplication, APIs like var aliasAnalysis can be factored out into a separate protocol (e.g. AnalysisProvider) to which both, FunctionPassContext and TestContext, can conform to.

[atrick]

Once you decide the test function should get a Context, you might as well give it a FunctionPassContext, which is the only one that makes sense for a FunctionTest.

I don't agree with that. FunctionPassContext has many APIs which are not relevant for a test and should not be called from a test. It's better to have a separate TestContext which conforms to Context or MutatingContext and only provides the APIs which are needed for testing. To avoid code duplication, APIs like var aliasAnalysis can be factored out into a separate protocol (e.g. AnalysisProvider) to which both, FunctionPassContext and TestContext, can conform to.

I don't see anything in FunctionPassContext that should not be called from a test.

Function tests always run in a function pass, and there's nothing to hide about that. The only difference is that we don't need a separate pass for all tests and they don't pollute the pass namespace. Those are important differences, but they don't change anything about the Context.

[eeckstein]

I don't see anything in FunctionPassContext that should not be called from a test.

A few examples:

continueWithNextSubpassRun
createSimplifyContext
optimizeMemoryAccesses
eliminateDeadAllocations
...

Why shouldn't we have a separate context for tests? It's easy to add one. And better than re-using an existing type which ends up with a "fat" interface because it has to serve two different purposes.
Maybe we want to add some test specific APIs to TestContext some day, e.g. APIs for performance testing, test selection, etc.

Also, when we move the test infrastructure to the SIL module at some day - including the basic Context type(s) - we cannot use FunctionPassContext anyway. (There shouldn't be a "pass" related type/name in the SIL module).

[atrick]

Why shouldn't we have a separate context for tests? It's easy to add one. And better than re-using an existing type which ends up with a "fat" interface because it has to serve two different purposes. Maybe we want to add some test specific APIs to TestContext some day, e.g. APIs for performance testing, test selection, etc.

Also, when we move the test infrastructure to the SIL module at some day - including the basic Context type(s) - we cannot use FunctionPassContext anyway. (There shouldn't be a "pass" related type/name in the SIL module).

You can pick out any of those APIs and say they don't make sense for a particular pass. Independent of this PR, it would make sense to split FunctionPassContext into something that should go in the SIL module. But I'm not going to resolve the module split debate in this PR. A FunctionTest runs in a function pass and needs to be able to do exactly anything that a function pass can do. I originally called it FunctionPassTest but Nate merged them for simplicity since we don't need anything in the SIL module today.

[atrick]

But I'm not going to resolve the module split debate in this PR.

Note that rewriting all of the utilities to take something other than FunctionPassContext is also way out of scope for this PR, and not something I think has any urgency.

[nate-chandler]

Yeah, for simplicity, there should¹ be "one thing" that's used to test utilities at function scope in the context of an IR function, namely a FunctionTest, whether your tests are in C++ or Swift.

While it's true that that name refers to different types depending on whether the test is in C++ or Swift, we shouldn't¹ burden people writing and testing utilities with thinking about that. Except to the extent that they already have to think about interface/architectural differences between C++ and Swift. Actually, the goal should¹ be to match those differences as much as possible so the test code uses the same idioms as the utility code being tested.

In both languages, the third argument is some gadget from which dependencies are retrieved. In Swift, where the currency type for such dependency containers exists and is some sort of Context/MutatingContext/FunctionPassContext, the third argument should¹ be of that type. In C++, where no such currency type exists, it makes as much sense as anything to reuse the FunctionTest type for this.

In the fullness of time, it might be nice to have a separate context type in the SIL module just for tests which grows abilities via extensions when used from downstream modules (e.g. Optimizer). In order to do that, the relevant protocols and types they expose would have to be sunk.

As it is, though, it's common for FunctionPassContext to appear as an argument to functions which we might want to test. So long as that's the case, it makes tests a bit cleaner¹ not to be provided a TestContext from which via computed property they would retrieve a FunctionPassContext which they then pass to the utility: myUtility(context.context) vs myUtility(context).

¹ In my opinion, currently.

[atrick]

@swift-ci test

[atrick]

As it is, though, it's common for FunctionPassContext to appear as an argument to functions which we might want to test. So long as that's the case, it makes tests a bit cleaner¹ not to be provided a TestContext from which via computed property they would retrieve a FunctionPassContext which they then pass to the utility: myUtility(context.context) vs myUtility(context).

¹ In my opinion, currently.

Utilities and tests should have the same context. There can be a separate discussion about all the different types of contexts we might want. But we need to be able to write tests against all of them, and the tests should take exactly the same context as the utilities.

[eeckstein]

[...] it makes tests a bit cleaner¹ not to be provided a TestContext from which via computed property they would retrieve a FunctionPassContext which they then pass to the utility

Yes, that would be terrible

Utilities and tests should have the same context. There can be a separate discussion about all the different types of contexts we might want.

I designed the context types with following idea in mind:
Context and MutatingContext are protocols. Whereas FunctionPassContext et al are concrete types which are provided to the entry points of passes. They conform to those context protocol(s).
Utilities (especially utilities which will end up in the SIL module) should not work on a concrete context type, but should be generic on some context protocol(s), depending on what they want to do. For example, a utility which modifies SIL needs to get some MutatingContext. You can search for some Context or some MutatingContext for examples.

This is easily extendible. For example if we add utilities which need analysis, we could add an AnalysisContext protocol, providing var dominatorTree, etc. And of course, FunctionPassContext would conform to this protocol.

As I said before, it makes sense to move the Context and MutatingContext protocols, together with a bunch of basic utilities, to the SIL module.

Following this design, it makes sense to create a concrete TestContext which conforms to all the required protocols and is passed to the test functions. It will be very similar to FunctionPassContext, but not identical, e.g. it would not provide things like continueWithNextSubpassRun.
IMO, the SIL module should not be aware that there exists something like a "function pass". So even the name FunctionPassContext shouldn't appear in the SIL module's source code.

[eeckstein]

To be clear: I'm suggesting this for a follow-up work, not for this PR

[eeckstein]

lgtm!

[atrick]

Following this design, it makes sense to create a concrete TestContext which conforms to all the required protocols and is passed to the test functions. It will be very similar to FunctionPassContext, but not identical, e.g. it would not provide things like continueWithNextSubpassRun.

The one essential requirement is that tests get the same context as utilities. Currently, utilities take FunctionPassContext. Everything could be changed to take a FunctionContext protocol instead.

A FunctionTest does run in function pass though, so it makes sense that it gets a FunctionPassContext. I see no reason we wouldn't want to test continueWithNextSubpassRun. There's nothing a test needs that is not already provided by FunctionPassContext. So this doesn't make the interface any "fatter" than it already is.

We might want some other kind of test that cannot assume that it runs in a function pass. But then we will have multiple test types. That's fine with me, but when I tried to do that in this PR there was an objection.

[eeckstein]

Currently, utilities take FunctionPassContext.

No, utilities should take a context protocol, like some Context or some MutatingContext. And most utilities already do (with a few exceptions in OptUtils).

Everything could be changed to take a FunctionContext protocol instead.

I'm not sure what a FunctionContext should be.

The one essential requirement is that tests get the same context as utilities.

This is given by TestContext conforming to all the required protocols.

[atrick]

Why does FunctionPassContext have it's own API surface if we aren't supposed to use the type?

Every API provided by FunctionPassContext should be testable. So whatever Context interface is passed to function tests should have those APIs. There's no reason to create a distinction between code that can run in a test vs. code that can run in a pass.

The implementation of the function test runner is a function pass. So the implementation of whatever Context interface we pass to function tests should be a FunctionPassContext.

I'm not sure what a FunctionContext should be.

FunctionContext is a protocol that provides the interface currently defined in FunctionPassContext. I don't see any reason to use a protocol, but that's what you were asking for.