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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

> 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.

I'm not sure I follow. I think we're on the same page about wanting to fix CodeExtractor's handling of `token` inputs, but I'm not sure why a separate patch would be needed to do that.

> Maybe i misunderstood but keeping the first call as above, and removing the later call (line 1442) results in the following failure. Moving findInputsOutputs anywhere before splitting PHI, or splitting return could result in miscompilation.

Oh, on this point I think we're //not// on the same page :). In addition to moving the call to `findInputsOutputs` to beginning of `extractCodeRegion` (where input validation must happen), I'm suggesting that we modify `extractCodeRegion` to //update// its `input` and `output` value sets as it modifies IR. I think I've outlined all the steps needed to do that in my previous comment. This would be simpler and cheaper than recomputing inputs & outputs from scratch a third time -- a side benefit is that it would probably make the phi splitting code clearer as well.


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

https://reviews.llvm.org/D68192