[flang][OpenMP][DoConcurrent] Simplify loop-nest detection logic #199
Conversation
ergawy
force-pushed
the
simplify_loop_nest_detection
branch
from
3c5e819 to
77ce502
Compare
ergawy
force-pushed
the
simplify_loop_nest_detection
branch
from
77ce502 to
08aacb4
Compare
| mlir::omp::MapInfoOp createMapInfoOp( | ||
| mlir::OpBuilder &builder, mlir::Location loc, mlir::Value baseAddr, | ||
| mlir::Value varPtrPtr, std::string name, llvm::ArrayRef<mlir::Value> bounds, | ||
| llvm::ArrayRef<mlir::Value> members, mlir::ArrayAttr membersIndex, | ||
| uint64_t mapType, mlir::omp::VariableCaptureKind mapCaptureType, | ||
| mlir::Type retTy, bool partialMap = false) { |
There was a problem hiding this comment.
Been staring at this for minutes looking for what changed
There was a problem hiding this comment.
Nothing, it was just formatted incorrectly, I think.
| if (isIndVarUltimateOperand(operand->getOwner(), doLoop)) | ||
| result = operand->get(); |
There was a problem hiding this comment.
What is supposed to happen when multiple match? An early exit doesn't seem possible, but can you add an assert(!result && "loop can have only one induction variable"), respectively comment about that an arbitrary one is selected?
There was a problem hiding this comment.
Only one should match since fir::DoLoopOp is defined as such. Will add an assertion thoug.
| mlir::BackwardSliceOptions backwardSliceOptions; | ||
| backwardSliceOptions.inclusive = true; | ||
| // We will collect the backward slices for innerLoop's LB, UB, and step. | ||
| // However, we want to limit the scope of these slices to the scope of | ||
| // outerLoop's region. | ||
| backwardSliceOptions.filter = [&](mlir::Operation *op) { | ||
| return !mlir::areValuesDefinedAbove(op->getResults(), | ||
| outerLoop.getRegion()); | ||
| }; |
There was a problem hiding this comment.
The backwardSliceOptions is not used anywhere?
There was a problem hiding this comment.
Luckily not anymore. We don't have to extract the slices relevant to innerLoop's LB, UB, and step since they are now hoised for loop nests by llvm#114020.
| if (op == innerLoop) | ||
| return mlir::WalkResult::skip(); | ||
|
|
||
| if (op->hasTrait<mlir::OpTrait::IsTerminator>()) |
There was a problem hiding this comment.
Is any terminator allowed? Limit to DoLoop's terminator?
| /// | ||
| /// 1. the operations needed to assing/update \p outerLoop's induction variable. | ||
| /// 2. \p innerLoop itself. | ||
| /// |
There was a problem hiding this comment.
I think we also need to test:
- Computing the inner DoLoop's lb/ub/step does not depend on the outer loop's indvar
- No instructions that have side-effects
[idea] It would be possible to just test whether the instructions can be sunk into the inner loop: it is not really relevant whether those instructions depend on the outer loop's indvar.
There was a problem hiding this comment.
Computing the inner DoLoop's lb/ub/step does not depend on the outer loop's indvar
For loop nests (i.e. multi-range do concurrent loops) this is prohibited by the spec:
C1125 (R1126) A concurrent-limit or concurrent-step in a concurrent-control shall not contain a reference to
any index-name in the concurrent-control-list in which it appears.
And we emit conforming loop nests now after llvm#114020 so no need to check for that (this is what the backward slices were used for in the earlier PR).
No instructions that have side-effects
You mean sibling instructions to the inner loop (i.e. either before or after it within the outer loop)? Or instruction within the body of the innermost loop in the nest?
ergawy
force-pushed
the
simplify_loop_nest_detection
branch
2 times, most recently
from
d0e4fa2 to
cb579d3
Compare
With llvm#114020, do-concurrent loop-nests are more conforment to the spec and easier to detect. All we need to do is to check that the only operations inside `loop A` which perfectly wraps `loop B` are: * the operations needed to update `loop A`'s iteration variable and * `loop B` itself. This PR simlifies the pass a bit using the above logic and replaces ROCm#127.
ergawy
force-pushed
the
simplify_loop_nest_detection
branch
from
cb579d3 to
8e6a70e
Compare
|
Ping! Please take a look when you have time. |
3 participants
With llvm#114020, do-concurrent loop-nests are more conforment to the spec and easier to detect. All we need to do is to check that the only operations inside
loop Awhich perfectly wrapsloop Bare:loop A's iteration variable andloop Bitself.This PR simlifies the pass a bit using the above logic and replaces #127.