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

[Arnamoy B via Phabricator via llvm-commits]

arnamoy10 added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:3045
+  if (!(Simdlen == nullptr && Safelen == nullptr)) {
+    ConstantInt *VectorizeWidth = Simdlen == nullptr ? Safelen : Simdlen;
     addLoopMetadata(
----------------
psoni2628 wrote:
> domada wrote:
> > Could you add comment which describes how simdlen and safelen clauses work?
> > 
> > BTW. Have you checked invalid test case where safelen < simdlen? 
> 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");
> +  }
> 
> ```
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131526