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

[Akash Banerjee via flang-commits]

TIFitis wrote:

I think my understanding of the bounds is different from what is being suggested by @agozillon and @kiranchandramohan.

I'll try to summarise my view below, please correct my where I am wrong.

1. `bounds-X` -> This is the bounds that is coming from the `box` values and persists through shape information in FIR/HLFIR etc.
2. `bounds-y` -> This is the bounds information that is optionally supplied by the user through the pragma. Eg. !$omp target map(a(5:10)).

`bounds-x` is something that is coming from the language itself and is used to generate code, such as indices for llvm-GEP instructions and what not.

`bounds-y` is supplied by the user, and from what I can tell the only code gen it affects is the offload_sizes, and the base_ptrs etc. that are passed on to the OMPRTL kernel calls.

The bounds information associated to `map_info` that @agozillon worked on is designed to capture `bounds-y`. And if I'm correct, although, some of the information for bounds-y may be generated using the bounds-x, they aren't directly correlated. Moreover, they are not interchangeable.

In my current implementation, I haven't added the bounds field to the `map_info` for `implicit` arguments, as obviously, there is no `bounds-y` associated with them which we might want to retain.

I do see that for something like `!$omp target map(a : alloc)`, which is a similar case to implicitly capturing `a`, the `boundsOp` is still generated for the `map_info`. @agozillon Can you please clarify why we might need the bounds information here? And if we do indeed need it then I can add the same for implicitly captured variables as well.

Now to summarise, it seems incorrect to me to store `bounds-x` information into the bounds field for `map_info`. And by the time MLIR is lowered to LLVM dialect, the shape information is no longer directly associated with the `alloca`. So, the mapping between the `block_args` and `bounds-x` is lost.

The following two seem feasible options to me:

- We introduce a new variable to the targetOp just for holding the mapping between the `block_args` to their respective `MLIR values`. We can justify it because of the `IsolatedFromAbove` trait which forces us to have everything as block_args.
- We continue to use the map_operands field to hold this information. Technically it works, and we can add some clever code when lowering from `MLIR` to `LLVMIR` to dispose off these unnecessary block_args and their associated map_operands.

Overall design-wise I prefer the first option. Seems like a cleaner way of handling block_args to me.

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