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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
