[flang][OpenMP] Extend do concurrent mapping to device.

[ergawy]

For simple loops, we can now choose to map do concurrent to either the host (i.e. omp parallel do) or the device (i.e. omp target teams distribute parallel do).

In order to use this from flang-new, you can pass: -fdo-concurrent-parallel=[none|host|device].

[mjklemm]

LGTM

[ergawy]

Thanks @mjklemm for the review.

Had some difficulty merging with ATD, but now merged and should be ready for review again in case somebody else in addition to Michael has time for review.

[skatrak]

Thank you Kareem for this work. I seem to be a bit too slow today to actually understand everything that's going on here, but I left you some of my thoughts going through it. In any case, it looks like a good step to get there. I'll probably better leave it to @agozillon to comment on the map-related part of the PR.

One thing to note is that this approach will have to change a bit with the next merge from main, which will hopefully already bring the changes to omp.wsloop. The main conflict with your work will be that omp.wsloop %i... { body } will have to be replaced by omp.wsloop { omp.loop_nest %i... { body } } and no operations will be allowed between the nested omp.distribute, omp.parallel, omp.wsloop and omp.loop_nest operations because they would be loop wrappers representing a composite construct. I'm not sure how long it will be before that happens, but I'm hoping for about a week, depending on reviews and so on. If it's not too much of a blocker for you, I'd prefer if you could wait until then before merging this (even if it's ready before), so that any conflicts specific to your PR can be addressed separately, I could help your with that as well.

[skatrak]

Nit: I suppose moving flang/include/Lower/OpenMP.h to flang/include/Lower/OpenMP/OpenMP.h would be in order as well (just to keep things consistent between the lib/ and include/ directories). If you do, remember to update the include guard on that file.

[ergawy]

To reduce the pain for anyone merging from upstream, I prefer to avoid doing that for now. Maybe we should replicate the header reorganization upstream first?

[skatrak]

I think it makes sense to copy the ops related to constants and type casts into the target region (rather than just before omp.wsloop, since that means introducing operations between the leaf constructs of a composite, which will be disallowed by loop wrappers), but shouldn't everything be already mapped and present as an entry block argument to omp.target by this time, due to it being detected as a live-in value?

[agozillon]

I think one issue that may arise, is that map operations only take pointer-like values, Index values are not mappable (you can circumvent this by creating the right operations to load it into memory (so convert it to a ref) and pass that instead I would imagine), there was some contention with this when @TIFitis tried to do it originally, so we rolled back to just using pointer-like arguments in map operations.

[ergawy]

but shouldn't everything be already mapped and present as an entry block argument to omp.target by this time, due to it being detected as a live-in value?

For the simple cases I tested so far, this is not the situation since LB, UB, step and constants; so they are not detected as live-ins. Only the induction variable is a live-in.

I think it makes sense to copy the ops related to constants and type casts into the target region (rather than just before omp.wsloop,

If you don't mind, I will leave that until we merge your work on composite constructs from upstream.

[agozillon]

LGTM, some minor comments here and there, mostly just some info dumping and two nits, one of which is up to you what you'd like to do :-)

Of course, please do wait on @skatrak's approval before going forward!

[agozillon]

I'm not against changing this to the more generic mlir::OpBuilder, but I am wondering if we should just keep it the same as most of the other flang utilities to keep it somewhat standardized, but I'll leave that up to you!

[ergawy]

That will come a small additional cost of having to replicate FirOpBuilder::createIntegerConstant's logic. Not much work but I would prefer to keep the FirOpBuilder parameter.

[agozillon]

You can likely verify the calculation in the DirectiveCommons.h code where the bounds are generated, and if still in doubt fire a message towards one of the OpenACC guys who created it, there's been a bug or two before, but they're generally quite good at clarifying things :-)

But I suppose if the lower bound is 0, and the extent is the max size of a dimension, you'd get the upper bound, but the subtraction seems like extra legwork at a minimum here I think, as you could just take the extent I would imagine.

[ergawy]

Makes sense. Thanks for pointing out who to contact. I will leave it for now until we hit the (potential) issue.

[agozillon]

The map code looks reasonable for the moment! I would just advise that you make sure this pass occurs prior to the OpenMP descriptor pass otherwise the descriptor types (allocatables et al) that are captured won't be expanded appropriately which will result in them not being mapped correctly. I think that should be enough to have these types mapped implicitly.

You probably already have the passes pipelined like this but just wanted to make sure!

[agozillon]

This little segment of code will also need to maintain lockstep with any changes to the implicit mapping in the OpenMP lowering as well unfortunately. There is nothing new yet, but there likely will be in the future but I can perhaps update this or keep you informed of any changes I make (or I see being made) :-)

[ergawy]

This little segment of code will also need to maintain lockstep ...

Unfortunately, yes. I don't really like that as we discussed on Teams awhile back 😞. I think we might have to reopen that discussion in the future and try to find a way that bridges the gap between the PFT and MLIR worlds to allow code reuse here.

[ergawy]

I would just advise that you make sure this pass occurs prior to the OpenMP descriptor pass otherwise the descriptor types (allocatables et al) that are captured won't be expanded appropriately which will result in them not being mapped correctly.

Thanks for pointing that out. I switched the 2 passes since the do concurrent pass was originally added after the OMP descriptor one.

[agozillon]

seems reasonable from what I can see, but I would also keep in mind that during the generation of Map <-> BlockArgs that occurs, we also have a little segment of code that clones bounds into the target region where possible (I think genLiveInDeclare may be trying to do something similar but please do correct me if I am wrong), and if it can't I believe it generates a map for the bounds, but @TIFitis is more familiar with this and can perhaps give further input or answer questions you might have on it!

[ergawy]

(I think genLiveInDeclare may be trying to do something similar but please do correct me if I am wrong)

Indeed that's what happens now. The relevant part of genLiveInDeclare is:

          mlir::Operation *localExtentDef =
              boundsOp.getExtent().getDefiningOp()->clone();
          rewriter.getInsertionBlock()->push_back(localExtentDef);

[agozillon]

I think one issue that may arise, is that map operations only take pointer-like values, Index values are not mappable (you can circumvent this by creating the right operations to load it into memory (so convert it to a ref) and pass that instead I would imagine), there was some contention with this when @TIFitis tried to do it originally, so we rolled back to just using pointer-like arguments in map operations.

[ergawy]

Thanks @skatrak & @agozillon for the review.

[ergawy]

That will come a small additional cost of having to replicate FirOpBuilder::createIntegerConstant's logic. Not much work but I would prefer to keep the FirOpBuilder parameter.

[ergawy]

To reduce the pain for anyone merging from upstream, I prefer to avoid doing that for now. Maybe we should replicate the header reorganization upstream first?

[ergawy]

Makes sense. Thanks for pointing out who to contact. I will leave it for now until we hit the (potential) issue.

[ergawy]

This little segment of code will also need to maintain lockstep ...

Unfortunately, yes. I don't really like that as we discussed on Teams awhile back 😞. I think we might have to reopen that discussion in the future and try to find a way that bridges the gap between the PFT and MLIR worlds to allow code reuse here.

[ergawy]

I would just advise that you make sure this pass occurs prior to the OpenMP descriptor pass otherwise the descriptor types (allocatables et al) that are captured won't be expanded appropriately which will result in them not being mapped correctly.

Thanks for pointing that out. I switched the 2 passes since the do concurrent pass was originally added after the OMP descriptor one.

[ergawy]

(I think genLiveInDeclare may be trying to do something similar but please do correct me if I am wrong)

Indeed that's what happens now. The relevant part of genLiveInDeclare is:

          mlir::Operation *localExtentDef =
              boundsOp.getExtent().getDefiningOp()->clone();
          rewriter.getInsertionBlock()->push_back(localExtentDef);

[ergawy]

but shouldn't everything be already mapped and present as an entry block argument to omp.target by this time, due to it being detected as a live-in value?

For the simple cases I tested so far, this is not the situation since LB, UB, step and constants; so they are not detected as live-ins. Only the induction variable is a live-in.

I think it makes sense to copy the ops related to constants and type casts into the target region (rather than just before omp.wsloop,

If you don't mind, I will leave that until we merge your work on composite constructs from upstream.

[skatrak]

Sorry for the delay getting back to this, but thank you Kareem for working on previous feedback and adapting the patch to the changes to OpenMP loop representations. I've got a few comments mostly to reorganize things a bit and some nits, but nothing substantial.

[skatrak]

Looks great, thank you!