Make `ApplyInst` a `MultipleValueInstruction`?

Development Compiler
,
3

@Andrew_Trick @Erik_Eckstein

Thanks for bringing this up! Actually there is a bigger history here. Originally SIL had multiple instruction results. There was a lot of horrible code written that did not handle multiple instruction results correctly. At the time we did not have any need for multiple instruction results, So rather than fix that it was decided that we were going to rip out multiple instruction results from SIL. This was done and a bunch of bugs were fixed. Eventually we realized that we actually needed this functionality for destructures and co-routines, so we put support back in, but we added the minimum support necessary so that we could use it where we needed to. Changing ApplyInst in this way is going to involve I think adding support for MultipleValueInstruction to many more parts of the compiler.

That being said:

  1. Adding more support for MultipleValueInstruction to the compiler is good!
  2. ApplyInst is still how it was from that first transition from multiple instruction results -> single instruction results. It originally had multiple results IIRC.
  3. The main challenge here is going to be adding support for MultipleValueInstruction in a bunch of passes in the compiler and making sure things do not regress as a result of the change.
  4. The only reason that I don't think this has been done is that it is a medium sized project since it would involve touching a bunch of code and no one has had time.

The main question I think is how to stage it in without disrupting the rest of the project. What are your thoughts on that @dan-zheng. My main concern would be that one large patch with a flag day will make the change really hard to review. Instead I wonder if we could stage it in like this:

  1. Change ApplyInst to a MultipleValueInstruction... but only have SILGen emit ApplyInst's with a single result and leave in the tuple extraction codegen. Then the underlying codegen doesn't change and even Textual SIL is unchanged (IIRC we don't () around multiple instruction result arrays with a single value). And you can then use that to discover the passes/IRGen/etc that need to be updated. The main thing will be changing APIs that only accept SingleValueInstruction to instead accept SILInstruction. I would look for any place where code assumes a single value instruction and then does a pattern match against AppllyInst. These will start to fail after this change. The best part is since you are using SILInstruction, these changes can be committed without changing how SILGen emits code! You can also add support for destructure in certain places to add more tests if you want.

  2. Then change SILGen to emit the ApplyInst's with multiple instruction results and eliminate the tuple extraction codegen. This is going to cause more SIL tests to fail and will probably require a bunch of test updates. But if one did the first part right, it should be significantly easier since one has taken care of a lot of the pass updates early. Also most likely one will be familiar with the passes that need to be updated due to the work one did in part 1.

Your thoughts?

1 Like