[MLIR][OpenMP] Lower only target related pragmas for GPU MLIR #40
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -127,6 +127,46 @@ findAllocaInsertPoint(llvm::IRBuilderBase &builder, | |||||
| &funcEntryBlock, funcEntryBlock.getFirstInsertionPt()); | ||||||
| } | ||||||
|
|
||||||
| static bool isOpAllowedToBeLowered(Operation *opInst, | ||||||
| llvm::OpenMPIRBuilder *ompBuilder) { | ||||||
| if (!opInst) | ||||||
| return false; | ||||||
|
Comment on lines
+132
to
+133
There was a problem hiding this comment. I think this wouldn't be necessary, since the original code doesn't check this. |
||||||
| // omp.target operation can be lowered for host and device MLIR | ||||||
| if (isa<omp::TargetOp>(opInst)) | ||||||
| return true; | ||||||
|
|
||||||
| // OpenMP operations inside omp.target can be lowered for host and device MLIR | ||||||
| if (opInst->getParentOfType<omp::TargetOp>()) | ||||||
| return true; | ||||||
|
|
||||||
| // TODO: Add support for test case: | ||||||
| // omp.parallel { //host pragma | ||||||
| // omp.target { } | ||||||
| // } | ||||||
| bool hasTargetRegion = | ||||||
| opInst->walk([](omp::TargetOp) { return WalkResult::interrupt(); }) | ||||||
| .wasInterrupted(); | ||||||
| if (hasTargetRegion) | ||||||
| opInst->emitError("Target region inside other pragma is not yet supported"); | ||||||
|
Comment on lines
+146
to
+150
There was a problem hiding this comment. I guess this should be checked before the previous check that looks for a parent omp.target {
omp.teams {
omp.target { }
}
}That kind of thing might eventually appear for reverse-offload regions, though we're a long way off from supporting that yet. There was a problem hiding this comment. Question for @skatrak - What is the meaning of the a There was a problem hiding this comment. Right, what happens when you enter an OpenMP target region is that execution moves to a target device. If a target device encounters a target region annotated with the Since it's not implemented yet, we can't be 100% sure of how we're going to represent this in MLIR, but I think it's likely that we would just follow the way it's done in the source and get something similar to this: // runs in the host...
omp.target {
// runs in the device...
omp.target device(ancestor = 1) {
// runs in the host...
}
// continues running in the device...
}
// continues running in the host...This is where I think nested There was a problem hiding this comment. Thanks, I hope applications that do this have kernels that are big enough to make it worthwhile to do all the shuttling between host and device (or the cost of this shuttling is kept really low through zero-copy buffers and such things but still there can be such a thing as too much shuttling I think). |
||||||
|
|
||||||
| // Check if given OpenMP operation belongs to function labelled with | ||||||
| // omp declare target pragma | ||||||
| LLVM::LLVMFuncOp funcOp = opInst->getParentOfType<LLVM::LLVMFuncOp>(); | ||||||
| omp::DeclareTargetDeviceType declareType = omp::DeclareTargetDeviceType::host; | ||||||
|
|
||||||
| if (!funcOp) | ||||||
| return false; | ||||||
|
There was a problem hiding this comment. Maybe the default if it's not inside of a function should be to lower it rather than ignoring it.
Suggested change
|
||||||
| auto declareTargetOp = | ||||||
| dyn_cast<omp::DeclareTargetInterface>(funcOp.getOperation()); | ||||||
| if (declareTargetOp && declareTargetOp.isDeclareTarget()) | ||||||
| declareType = declareTargetOp.getDeclareTargetDeviceType(); | ||||||
| if ((declareType == omp::DeclareTargetDeviceType::host) && | ||||||
| ompBuilder->Config.isGPU()) { | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| return false; | ||||||
| } | ||||||
| return true; | ||||||
| } | ||||||
|
|
||||||
| /// Converts the given region that appears within an OpenMP dialect operation to | ||||||
| /// LLVM IR, creating a branch from the `sourceBlock` to the entry block of the | ||||||
| /// region, and a branch from any block with an successor-less OpenMP terminator | ||||||
|
|
@@ -3182,6 +3222,9 @@ LogicalResult OpenMPDialectLLVMIRTranslationInterface::convertOperation( | |||||
|
|
||||||
| llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder(); | ||||||
|
|
||||||
| // Skip lowering of an OpenMP operation if it's context is not appropriate | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| if (!isOpAllowedToBeLowered(op, ompBuilder)) | ||||||
| return success(); | ||||||
| return llvm::TypeSwitch<Operation *, LogicalResult>(op) | ||||||
| .Case([&](omp::BarrierOp) { | ||||||
| ompBuilder->createBarrier(builder.saveIP(), llvm::omp::OMPD_barrier); | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Saying an operation is "not allowed" to be lowered I think implies that it should be an error, which is not the intent. Maybe we can rename this to something more along the lines of "operation should be skipped" or "operation not applicable" or something like that. It doesn't have to be the name I suggest, but something along those lines.
If you agree to make this name change, note that the logic would be reversed.