[OpenMP][flang][do-conc] Map values depending on values defined outside target region. #148
Conversation
ergawy
force-pushed
the
do_concurrent_runtime_sized_arrays
branch
from
dde321c
to
f434fdc
Compare
ergawy
changed the title
| while (!valuesDefinedAbove.empty()) { | ||
| for (mlir::Value val : valuesDefinedAbove) { | ||
| mlir::Operation *valOp = val.getDefiningOp(); | ||
| if (mlir::isMemoryEffectFree(valOp)) { |
There was a problem hiding this comment.
The case where the value defined above is a block argument (valOp == nullptr) will cause the compiler to crash during this check. If there is a reason why that's never supposed to happen, I'd suggest adding a specific assert for it. Otherwise, I think that case should be handled.
There was a problem hiding this comment.
This is copied from OpenMP.cpp and will extracted to a shared util later (see TODO above the function). So I assume we did not hit problems with this yet. But I agree with you, I added an assert here and OpenMP.cpp to be on the safe side.
| mlir::Operation *clonedOp = valOp->clone(); | ||
| regionBlock->push_front(clonedOp); | ||
| val.replaceUsesWithIf( | ||
| clonedOp->getResult(0), [regionBlock](mlir::OpOperand &use) { |
There was a problem hiding this comment.
If valOp defines multiple values and val is not the first, clonedOp->getResult(0) won't be the right value to use here. This probably needs a bit of refinement to find the right index. Another issue is that, if multiple values defined by the same operation above are used inside the target region, it will result in the same operation being cloned multiple times.
| llvm::SmallVector<mlir::Value> bounds; | ||
| std::stringstream name; | ||
| builder.setInsertionPoint(targetOp); | ||
| mlir::Value mapOp = createMapInfoOp( |
There was a problem hiding this comment.
Nit: Would it be possible to use genMapInfoOpForLiveIn, perhaps adapting it if needed? I'm just wondering this code seems duplicated and we'd eventually need to support all sorts of types in both cases.
There was a problem hiding this comment.
I want to keep both this and the corresponding Lower/OpenMP.cpp code as close as possible to extract into a shared util (hopefully soon once I am done with getting LBL's codebase to work for the device).
…de target region. Adds support for `do concurrent` mapping when mapped value(s) depend on values defined outside the target region; e.g. the size of the array is dynamic. This needs to be handled by localizing these region outsiders by either cloning them in the region or in case we cannot do that, map them and use the mapped values.
ergawy
force-pushed
the
do_concurrent_runtime_sized_arrays
branch
from
f434fdc
to
93c1ec3
Compare
Adds support for
do concurrentmapping when mapped value(s) depend on values defined outside the target region; e.g. the size of the array is dynamic. This needs to be handled by localizing these region outsiders by either cloning them in the region or in case we cannot do that, map them and use the mapped values.