[PATCH] D68192: Fix PR40710: Outlined Function has token parameter but isn't an intrinsic

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:1339
+  ValueSet inputs, outputs, SinkingCands, HoistingCands;
+  findInputsOutputs(inputs, outputs, SinkingCands, true);
+
----------------
On closer inspection, I don't think my suggestion for storing an 'are inputs eligible' bit inside of CodeExtractor after `findInputsOutputs` runs the first time is a great idea (and I see that you haven't followed that advice here). The problem with that approach is that it requires clients to run `findInputsOutputs`, not modify any IR, and then call `extractCodeRegion`. That happens to be how clients use CodeExtractor today, but the new requirement could be brittle.

That said, I don't think the solution can be to run `findInputsOutputs` a third time. In addition to being expensive, it creates some confusion over the need for the operation to be repeated three times.

I think there's a simpler and cheaper alternative: run `findInputsOutputs()` exactly once within `extractCodeRegion()`. Note that `splitReturnBlocks()` doesn't change the input set. In `severSplitPHINodesOfEntry()`, you can add phis from the old header block to the input set if they have uses in the extraction region. Similarly, `severSplitPHINodesOfExits()` can be modified to remove exit block phis from the output set, and to add any newly created split phis.

To recap, please call `findInputsOutputs()` just once at the start of `extractCodeRegion()` and delete the subsequent call. Any input validation can happen up front (e.g. `findInputsOutputs` could return an llvm::Error indicating whether validation succeeded).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68192/new/

https://reviews.llvm.org/D68192