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

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya marked an inline comment as done.
hiraditya added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:1339
+  ValueSet inputs, outputs, SinkingCands, HoistingCands;
+  findInputsOutputs(inputs, outputs, SinkingCands, true);
+
----------------
vsk wrote:
> 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).
> 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.

One of the reasons why verification of input parameters could be a separate function and not called from isEligible here. The clients can make sanity checks before calling the extractCodeRegion. In general, any optimization pass, would have analysis phase and a transformation phase. IMHO This function should just extract the region and not do anything else. But there's already some check in place here which isn't ideal. The goal of this diff was to fix the ICE. I'm totally fine with repairing CodeExtractor in a separate patch.


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

https://reviews.llvm.org/D68192