[Mlir-commits] [mlir] [OpenMP][Flang] Add "IsolatedFromAbove" trait to omp.target (PR #67164)

[llvmlistbot at llvm.org]

agozillon wrote:

> @agozillon In the new test added that was initially shared by @jeanPerier , the bound is explicitly used in a mul op inside the region and is therefore also used in a mul instruction inside the region when lowered to llvmIR. Thus, we do need to map them as map_operands.

Yeah, they're nice cases and ones I haven't come across yet, so very good to have coverage for. 

> Most of the time however, the bounds are not explicitly used inside the region, in this case we will end up passing "unused" arguments to to the target execution. But, this is an optimisation problem. I don't know if there are existing optimisations in MLIR that remove unused block_args, should be fairly straightforward to add one.

Yeah that was my worry, but it is an optimisation issue, and on a bit of further thought, probably not too difficult to fix, you can likely tell what's being used and not used during lowering to LLVM-IR using getUsesDefinedAbove. I imagine you can't do something similar when you're creating the initial block arguments though unless the body of the target region is lowered already, and it's probably easier to do later in the pipeline in any case.  

> Can you explain what you mean by map_operands being occluded?

Just bad word usage from me saying it makes it a little noisy to tell what is actually being mapped vs not mapped if we hoist it all into maps, but as said in the previous paragraph, probably not too hard to solve in reality.

> When lowering from MLIR to llvmIR, anything that is part of the map_operands clause should be mapped as is, and there shouldn't be any variables inside the region which are unmapped, i.e, not present in the map_operands.
> 
> Thanks for sharing the tests, I'll run them tomorrow and update on their status.

No worries, hopefully they're helpful! Small sample set so far, but always good to add to it if you add something new that works at runtime you want to have tested.

As an aside is it possible to change the AnyType for the dialect operation to be something a little more restrictive such as AnyTypeOf<[IntLikeType, OpenMP_PointerLikeType]>, not a big deal but it means we'd catch accidental type usage a bit earlier and those are the only types we'd currently be looking to deal with in any case! 

Otherwise the patch LGTM so far. 

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