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

[Prabhdeep Soni via Phabricator via llvm-commits]

psoni2628 added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:625
+  void applySimd(CanonicalLoopInfo *Loop, Value *IfCond, ConstantInt *Simdlen,
+                 ConstantInt *Safelen);
 
----------------
shraiysh wrote:
> [nit] Please set the default value of Safelen to nullptr here.
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?


================
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(
----------------
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?


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

https://reviews.llvm.org/D131526