[PATCH] D90689: [CodeExtractor] Replace uses of extracted bitcasts in out-of-region lifetime markers

Wed Nov 4 09:41:13 PST 2020

vsk added subscribers: AndrewLitteken, paquette.
vsk added a comment.

In D90689#2371689 <https://reviews.llvm.org/D90689#2371689>, @ggeorgakoudis wrote:

> In D90689#2371522 <https://reviews.llvm.org/D90689#2371522>, @vsk wrote:
>
>> In the example, what pass/component is responsible for creating `%1 = bitcast i32* %0 to i8*`?
>>
>> If it's CodeExtractor itself, it would be nice to solve the problem earlier by getting rid of bitcasts which only exist to wire up lifetime markers. I tried doing that in 26ee8aff2 <https://reviews.llvm.org/rG26ee8aff2b85ee28a2b2d0b1860d878b512fbdef>, but had to revert (099bffe7f <https://reviews.llvm.org/rG099bffe7f7df41d66195ce33e91888a4a16c6b4a>) because llvm doesn't support specializing its lifetime intrinsics on opaque pointer types. That's a bug, I think: if we fix that, perhaps this whole problem goes away.
>>
>> OTOH, if this `%1 = bitcast i32* %0 to i8*` value comes from a frontend or another pass, I think the approach taken in this patch is reasonable. Please add a test (perhaps in the CodeExtractor unittest).
>
> Many thanks for the quick reply @vsk! The `bitcast` is generated by the frontend, not by CodeExtractor. Our use case is outlining a merged parallel region that includes sequential code which include bitcasts like that. I am going to add a unit test.
>
> I have two questions about the CodeExtractor.
>
> 1. I have second thoughts on changing the IR within the `findAllocas`. Should I create a helper data structure, e.g., `ReplaceBitcastUses` as in for `SinkCands`, and do the transformation inside the `extractCodeRegion` caller?
> 2. Is the assumption that `findAllocas` is always called before `findInputsOutputs`? If `findAllocas` is not called, my concern is that `findInputsOutputs` will report an output on the use of the `bitcast` although this output would be removed if `findAllocas` has been called. This problem does not appear for `extractCodeRegion` because it always calls `findAllocas` but there may be other modules using the CodeExtractor interface. I could change `findInputsOutputs` not to include `bitcast` uses to lifetime markers as outputs regardless of whether `findAllocas` has been called. Is that reasonable or does that create other problems?

The status quo is that findAllocas destructively updates the IR in ways that affect the results of findInputsOutputs: you're correct that this dependency can create issues. Imho CodeExtractor intermixes analysis and transformation too much. This makes it hard to improve its transformations (evidenced by your question (1)); it also makes it hard for client code to analyze extraction profitability, since you need to do the extraction to figure out what the real input/output set will be. (Before extractCodeRegions, you can query findInputsOutputs, but it's just guessing.)

Taking a short-term view, having findAllocas perform a new destructive update would get the job done and be in keeping with the current design. In the long term, I think it'd be nice to have a redesigned CodeExtractor that keeps analysis and transformation neatly separated. (that said, I'm not sure how best to achieve that (.. and I'm no longer actively working on it).)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90689