This is an archive of the discontinued LLVM Phabricator instance.

[OMPIRBuilder] Add support for safelen clause

Authored by psoni2628 on Aug 9 2022, 2:59 PM.



This patch adds OMPIRBuilder support for the safelen clause for the
simd directive.

Diff Detail

Event Timeline

psoni2628 created this revision.Aug 9 2022, 2:59 PM
Herald added a project: Restricted Project. · View Herald Transcript
psoni2628 requested review of this revision.Aug 9 2022, 2:59 PM
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2022, 3:00 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
domada added inline comments.Aug 10 2022, 1:41 AM

Could you update this comment?


Could you add comment which describes how simdlen and safelen clauses work?

BTW. Have you checked invalid test case where safelen < simdlen?

psoni2628 added inline comments.Aug 10 2022, 6:23 AM

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");
+  }
arnamoy10 added inline comments.Aug 10 2022, 6:32 AM

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.

psoni2628 updated this revision to Diff 451449.Aug 10 2022, 7:15 AM
  • Add comments based on reviewer's feedback
  • Rebase
Meinersbur added inline comments.Aug 10 2022, 7:54 AM

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) {
addLoopMetadata(CanonicalLoop, MDList);

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.

psoni2628 updated this revision to Diff 451500.Aug 10 2022, 8:47 AM
  • Add LoopMDList

Minor comments. Thank you for working on this!


[nit] Please set the default value of Safelen to nullptr here.


[nit] remove this change after setting default value.


[nit] same as above


[nit] same as above

psoni2628 added inline comments.Aug 14 2022, 7:39 PM

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?


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?

shraiysh added inline comments.Aug 15 2022, 4:00 AM

Sounds good to me.

Meinersbur accepted this revision.Aug 18 2022, 8:39 AM



Consider simplifying this expression


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.
This revision is now accepted and ready to land.Aug 18 2022, 8:39 AM
  • Simplify expression based on reviewer comments
  • Rebase
This revision was landed with ongoing or failed builds.Aug 18 2022, 12:44 PM
This revision was automatically updated to reflect the committed changes.