skatrak wrote:
> Passing callbacks around significantly adds complexity. Did you consider
> deriving a new class from CodeExtractor and overriding some method
> `createAlloca` for use by OpenMPIRBuilder?
> Overall, the approach makes sense to me. thank you for the PR. Only big
> concern is @Meinersbur's comment on using a derived class of `CodeExtractor`
> instead of callbacks. Having said that callbacks are all over the place in
> `OMPIRBuilder`.
Thank you @Meinersbur and @bhandarkar-pranav for the reviews, I'll try to
explain my though process on this so we can decide on a solution for this
problem.
The problem is that the logic inside of the allocator/deallocator will not
necessarily be the same for all custom instances. We could create an
OMPIRBuilder-local subclass of `CodeExtractor` holding a reference to the
OMPIRBuilder itself, and then subclass it for each case by overriding
allocation and deallocation methods. The issue then is that we'd have to know
at the point of creating the `CodeExtractor` from an `OutlineInfo` struct, in
`OMPIRBuilder::finalize`, which subclass (or base class) we would have to
instantiate. So, we'd have to extend `OutlineInfo` to hold the union of all
unique information needed to initialize each `OutlineInfo` subclass constructor
and some enumeration or something else that can be used to select which one to
create.
This patch only introduces a custom allocator for `parallel` when compiling for
a target device, which specifically uses `__kmpc_{alloc,free}_shared`, but in
my mind whatever we do needs to allow us to support other custom cases if they
arise (e.g. it calls other function(s) with different arguments, it applies to
different constructs, etc). It seems to me that callbacks are the simpler
solution in this case, though I get that OMPIRBuilder has some next-level
spaghetti execution going on as it is and I'm suggesting adding yet another use
for callbacks in it.
Let me know if you still think subclassing `CodeExtractor` is a better way
forward for this and I'll give it a try.
https://github.com/llvm/llvm-project/pull/150925
_______________________________________________
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
