This is a preliminary review with some top level feedback from having studied the IsApplicable function - please see attached in one of the comments some proposed tests for the HasEarlyReturn method of Function, and a different way to implement it.

This is a complex transformation so I expect we'll need a bit of iteration in the review process, but in general this is looking really good!

The transformation is looking very promising! Some changes required.

This is coming along really well!

Rather than inserting comments in the code, here is a patch with some changes to the way the transformation works that I suggest you study and then incorporate (editing it as you see fit, of course):

patch.txt

The main things I've done here are:

• Do require that the returned id - if defined in the function - is mapped. Now that we're handling a return via a copy object this is necessary

• Require that parameter ids are not mapped. I had a mistake in my test where I had mapped a parameter id and this led to an applicable transformation that then failed to apply properly (fixed in the patch)

• Handle insertion of the OpCopyObject for OpReturnValue while the OpReturnValue is being processed, and not later. That's cleaner for various reasons.

Can you apply these changes, and for my next review can you include tests to check the following (your existing tests might have this; I didn't look yet):

• A function that directly uses OpKill or OpUnreachable cannot be inlined - please open an issue for the fact that we can relax this in the future: we can inline functions that use OpKill/OpUnreachable if the call site is not in a continue construct, but let's not do that in this PR.

• A function that has multiple returns cannot be inlined

In the next review round I will look at the fuzzer pass.

This is a very nice transformation idea.

I've noticed some issues with the implementation and some tricky corner cases with the SPIR-V structured control-flow rules (which you are handling btw; I just left some comments regarding those).

And be sure to check Ally's previous review summary regarding OpKill and OpUnreachable.

This looks really good! And this is a hard transformation (as cloning multiple blocks is non-trivial). I think it is very close to being done, but I have left a few comments that need addressing. Although some are just minor comments about style.

This looks nice!

However, I'd suggest you refactor Apply method in the transformation since there is no telling what can break by storing blocks directly in the instruction list (I'd assume that dominator analysis won't work for inlined blocks).

I'd suggest you do the following:

1. Split the caller block in two: the first part containing all the instructions before the call and the second part containing all the instructions starting with a call onward (see opt::BasicBlock::SplitBasicBlock).
2. Start creating blocks to inline and insert them before the second block (opt::Function::InsertBasicBlockBefore).
3. Remove the OpFunctionCall instruction from the second block.
4. Add a terminator to the first block pointing to the entry block of the inlined function.

With this approach, I think you can remove the constraint that the call must be the last instruction before the OpBranch. Although, you would need to check that the caller block is not a loop header.

This is looking really good and is now very close!

Please see the comments that I just added requesting some (probably final) changes.

Thanks for making the changes.

This looks really good, and I'm happy to merge it!