This patch adds OMPIRBuilder support for the safelen clause for the
simd directive.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | ||
|---|---|---|
| 3042 | Yes, I have checked this case, the case doesn't make it to OMPIRBuilder, because the frontend errors. ./ex2.c:9:38: error: the value of 'simdlen' parameter must be less than or equal to the value of the 'safelen' parameter
#pragma omp simd safelen(2) simdlen(3)
~ ^
1 error generated.I was thinking about adding an assertion to assert that simdlen < safelen , but I wasn't sure that it made sense to do so when Clang is checking this anyways, and the existing codegen for simdlen/safelen also doesn't check this. + // If both simdlen and safelen clauses are specified, the value of the simdlen parameter must be less than or equal to the value of the safelen parameter.
+ if (Simdlen != nullptr && Safelen != nullptr && (Simdlen->getValue().ugt(Safelen->getValue()))) {
+ llvm_unreachable("Simdlen must be less than or to Safelen when both present");
+ } | |
| llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | ||
|---|---|---|
| 3042 | An assert is probably not necessary, as this is part of semantic check (as you identified to be done by the frontend). The check is not part of codegen. | |
| llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | ||
|---|---|---|
| 3029–3032 | Instead of calling addLoopMetadata repeatedly, consider collecting metadata in a SmallVector and call addLoopMetadata only once at the end. SmallVector<Metadata*> MDList;
if (Safelen == nullptr) {
...
MDList.push_back(MDNode::get(...));
}
...
addLoopMetadata(CanonicalLoop, MDList); | |
| 3045 | safelen should not mean the same as llvm.loop.vectorize.width. safelen could be unreasonably large to use as SIMD width or a non-power-of-2. That being said, it's what CGStmtOpenMP.cpp does as well and I don't know any better way. | |
Comment Actions
Minor comments. Thank you for working on this!
| llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h | ||
|---|---|---|
| 625 | [nit] Please set the default value of Safelen to nullptr here. | |
| llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp | ||
| 1808 | [nit] remove this change after setting default value. | |
| 1920 | [nit] same as above | |
| mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | ||
| 976 | [nit] same as above | |
| llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h | ||
|---|---|---|
| 625 | Setting a default value for only Safelen but not Simdlen or IfCond is a bit inconsistent and could potentially cause confusion in the future. I think it would make more sense to set default values for all the clause values. Maybe we could this separately in a different patch? | |
| llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | ||
| 3045 | IMO, this is more of a semantic analysis problem. For example, if it not legal to have a safelen that is a non-power-of-2, then Clang should not let this value proceed from semantic analysis. Maybe we could add a check in clang/lib/Sema/SemaOpenMP.cpp, and fix the problem for both OMPIRBuilder and the existing codegen support in CGStmtOpenMP.cpp in a different patch? | |
| llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h | ||
|---|---|---|
| 625 | Sounds good to me. | |
Comment Actions
LGTM
| llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | ||
|---|---|---|
| 3042 | Consider simplifying this expression | |
| 3045 | I think it's not a responsibility of Clang, but the optimizer. For instance, the information that "infinite" vector width is good (#pragma clang loop vectorize(assume_safety) and #pragma omp simd` without safelen) is passed as metadata (addSimdMetadata), but there is not metadata for conveying that only a specific vector length is safe. There are other issues than just power-of-2, such as
| |
Could you update this comment?