[flang-commits] [flang] [flang][OpenMP] Adapt OMPMapInfoFinalization to run on all top level ops (PR #93545)

[via flang-commits]

agozillon wrote:

> Thanks for taking a look!
> 

No problem, always happy to, sometimes I miss things in the flood of emails from LLVM so feel free to ping me if I don't review or answer something quickly enough!

> > However, one question I may have is, will this result in walks on the same MapInfoOp more than once? Might be a dumb question, but I am unfamiliar with addNestedPassToAllTopLevelOperations unfortunately.
> 
> It means it will run on other top level operations (currently only `fir.global`, `omp.declare_reduction`, `omp.private`) as well as `func.func`, as though those were also functions. I don't think this should cause the pass to run on the same map operation multiple times.

Thank you very much, then I don't think it should have any affect and will work like it did previously, as it was working on FuncOp before hand and just walking them for MapInfoOp's.

> 
> There's a good argument that this is a waste of time because we won't generate map ops inside of these operations. But I wanted the pass pipeline to be consistent and future proof. I expect that in practice the overhead shouldn't be too bad because only a minority of operations will be outside of `func.func` and MLIR can run the pass on different sibling operations in parallel.

We will for FuncOp's but the rest not so much, still good to align with the rest I think. The only downside I would imagine is it is a lot harder to swap this to a ModuleOp for testing (print/cout testing at least) and debugging, as I imagine it'd run in parallel still like before, just a lot harder to swap to a ModuleOp. But not a big issue and maybe (probably is) there's a way to make sure an execution only utilises a single thread regardless of the operation it's executed on! Just something I am keenly aware of as this pass is still very much a WIP in a lot of areas :-) 

But regardless, happy to sign off on this as the question was answered and it seems to me it will work like before, at least for the moment with the current listing of top level operations.

https://github.com/llvm/llvm-project/pull/93545