Page MenuHomePhabricator

[Clang][OpenMP] Frontend work for sections - D89671
Needs ReviewPublic

Authored by AMDChirag on Nov 9 2020, 1:03 AM.

Details

Summary

Primary Author: @anchu-rajendran
This patch is child of D89671, contains the clang
implementation to use the OpenMP IRBuilder's section
construct.

Diff Detail

Unit TestsFailed

TimeTest
590 msx64 debian > Clang.OpenMP::cancel_codegen.cpp
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -verify -fopenmp -fopenmp-version=45 -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - /mnt/disks/ssd0/agent/llvm-project/clang/test/OpenMP/cancel_codegen.cpp | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/OpenMP/cancel_codegen.cpp --check-prefixes=ALL,CHECK
310 msx64 windows > Clang.OpenMP::cancel_codegen.cpp
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\clang.exe -cc1 -internal-isystem c:\ws\w64\llvm-project\premerge-checks\build\lib\clang\12.0.0\include -nostdsysteminc -verify -fopenmp -fopenmp-version=45 -triple x86_64-apple-darwin13.4.0 -emit-llvm -o - C:\ws\w64\llvm-project\premerge-checks\clang\test\OpenMP\cancel_codegen.cpp | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\clang\test\OpenMP\cancel_codegen.cpp --check-prefixes=ALL,CHECK

Event Timeline

AMDChirag created this revision.Nov 9 2020, 1:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 9 2020, 1:03 AM
AMDChirag requested review of this revision.Nov 9 2020, 1:03 AM
AMDChirag retitled this revision from [LLVM][OpenMP] Frontend work for sections - D89671 to [Clang][OpenMP] Frontend work for sections - D89671.Nov 9 2020, 1:15 AM

The test case will also be added here.

Meinersbur added inline comments.Nov 9 2020, 3:23 PM
clang/lib/CodeGen/CGStmtOpenMP.cpp
3614–3616

Instead of duplicating it here, why not using OpenMPIRBuilder::BodyGenCallbackTy?

3638

In what situation would there be no #pragma omp section children?

3646

I think this is unnecessary; SmallVector converts implicitly to an ArrayRef

fghanim added a comment.EditedNov 10 2020, 1:58 PM

Thanks for doing this.

Please add proper lit test. you can find that under clang/test/OpenMP/sections_codegen.cpp. add a case for using the OMPBuilder.
As an example look in the same directory for parallel_codegen.cpp , master_codegen.cpp , critical_codegen.cpp

In case you need it; you can check that your changes works by doing ninja/make -check-clang-openmp

Edit:
NIT: Also please change the title of the patch to be more descriptive of the contents of the patch (e.g. [clang][openmp] using OpenmpIRBuilder to implement openmp sections ) or anything you choose :)

AMDChirag updated this revision to Diff 307022.Nov 23 2020, 3:16 AM

Fixed usage of BodyGenCallbackTy
Removed ArrayRef variable

AMDChirag marked 2 inline comments as done.Nov 23 2020, 3:17 AM
AMDChirag added inline comments.Nov 23 2020, 4:33 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
3638

You are correct - I could not find a scenario where CS would be a nullptr.
However, the original implementation EmitSections() handles the CS == nullptr case as well. Also, if CS is nullptr, the software will exit with an assertion failure anyway (CodeGenFunction::EmitStmt()).
Should I just remove the else block altogether? I think an assert statement instead, for checking CS == nullptr should suffice.

AMDChirag updated this revision to Diff 313303.EditedDec 22 2020, 5:11 AM
AMDChirag marked an inline comment as done.

Updated code according to the changes in LLVM side of things.

Test cases are still required, however.

AMDChirag updated this revision to Diff 313307.Dec 22 2020, 5:27 AM

Updated BGenCallbackTy to StorableBodyGenCallbackTy.

Ping. Please add the Lit test for this.

Added OMP delegating code for createSection (EmitOMPSectionDirective).
@fghanim working on clang lit test case.