[PATCH] D131526: [OMPIRBuilder] Add support for safelen clause

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur accepted this revision.
Meinersbur added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3042
+
+  if (!(Simdlen == nullptr && Safelen == nullptr)) {
+    // If both simdlen and safelen clauses are specified, the value of the
----------------
Consider simplifying this expression


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3048
+    // parameter. Therefore, use safelen only in the absence of simdlen.
+    ConstantInt *VectorizeWidth = Simdlen == nullptr ? Safelen : Simdlen;
     addLoopMetadata(
----------------
psoni2628 wrote:
> Meinersbur wrote:
> > `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.
> 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?
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
 * too-large safelen (e.g. 128 when the target only has simd instructions of length 4). 
 * A vector width of 2 having better performance even though `safelen(2)` is used.
 * The LoopVectorize will not vectorize at all if it cannot conclude the legality of it when only `!{!"llvm.loop.vectorize.width", i32 4}` is specified, but not also the information that the user guarantees that it is safe to do so.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131526/new/

https://reviews.llvm.org/D131526