Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flang][OpenMP] Added support for lowering OpenMP taskwait construct #280

Closed
wants to merge 224 commits into from

Conversation

SouraVX
Copy link
Collaborator

@SouraVX SouraVX commented Jul 16, 2020

This patch adds lowering support for OpenMP-4.5 taskwait construct to
OpenMP Dialect operations.

omp-taskwait.f90 this file remain downstream, rest of the patch can be upstreamed once reviewed.
Please review/approve this PR, don't merge it. As discussed we'll merge/review upstream first.

schweitzpgi and others added 30 commits July 9, 2020 09:07
fix the fallout from the pass manager changes in MLIR
lapack work
[audit] add back changes that were lost with merge

Work on adjustable arrays.

  - This requires analysis of local symbols.
  - To do that requires getting the symbol table (scope), which required
    threading the SemanticsContext for the main program.
  - Added sorting and sorrted symbol tables to the PFT.
  - Refactored temporaries.
  - Misc. refactoring to follow LLVM coding standard, etc.

part 2 - implement the lower of variables

With these changes, I can compile code like srot.f.

A couple minor bug fixes

implement format for data transfer statements

update the lapack list

add a check and TODO
improve the folder for convert on bools

DCE was broken. This adds back the Op traits to make it work correctly.
Also tightens and repairs two tests, resp.

Replace internal symbol map.

The internal symbol map had served its purpose and needs to be replaced with a
mapping structure that is more appropriate for Fortran variables, which can be
composed of a number of runtime values.  This new map will allow us to lower
more complicated entities correctly by tracking their component values.
* Generate FIR for a group of multi-way branches
 - arithmetic if
 - assigned goto (needs assign statement, which is also implemented)
 - computed goto
 - select case

* Review comment update.
fix compilation issue post merge

remove reference to deleted header
Add fir::isa_real and fir::isa_complex and rework FirOpBuilder::isCharacter
…ap for

lowering.  This piece primarily gets started in the direction of being able
to lower dynamic arrays and character types correctly.

There is still more work in fully leveraging this in the bridge, but all the
existing tests pass with this rewrite.

FIR changes:

This patch changes the FIR region operations to be more like those found in
the loop dialect.  The changes are driven by the desire to be able to convert
FIR into a register ssa form.  The changes are specifically:

  - fir.loop has been replaced by fir.do_loop. The new form has the same
    semantics (inclusive bounds, unordered), but can now carry ssa-values
    around the loop and return them as results to the parent.
  - fir.where has been replaced by fir.if. This new form is identical to
    the current loop.if operation.
  - fir.iterate_while has been added. This operation is very similar to
    fir.loop with the addition that it requires a single loop-carried bool
    value that signals an early-exit condition to the operation.  While that
    value is true, iterate_while will continue to iterate. When it becomes
    false, the loop is exited.
  - fir.result is the new terminator for the above ops to facilitate the
    carrying of ssa-values through block arguments (phi nodes).

Change the syntax for fir.iterate_while to make it clear that the iterate condition is required.
Fixes bugs with lowering of fir.iterate_while to CFG.  The lowered code produced is now as follows.

    %0 = llvm.mlir.constant(1 : index) : !llvm.i64
    %1 = llvm.mlir.constant(100 : index) : !llvm.i64
    llvm.br ^bb1(%0, %arg0, %arg1 : !llvm.i64, !llvm.i1, !llvm.i32)
  ^bb1(%2: !llvm.i64, %3: !llvm.i1, %4: !llvm.i32):	// 2 preds: ^bb0, ^bb2
    %5 = llvm.icmp "slt" %2, %1 : !llvm.i64
    %6 = llvm.and %5, %3 : !llvm.i1
    llvm.cond_br %6, ^bb2, ^bb3
  ^bb2:	// pred: ^bb1
    %7 = ... : !llvm.i1
    %8 = llvm.add %2, %0 : !llvm.i64
    llvm.br ^bb1(%8, %7, %4 : !llvm.i64, !llvm.i1, !llvm.i32)
  ^bb3:	// pred: ^bb1

[review comment] improve description text to make it more clear

[review] minor cleanups per review comments
1. Initialize scalar intrinsic types
2. Add test global inits.
3. Add TODO place holders for things that need to be implemented
speculatively. This eliminates creation of some empty fir.if blocks.

Fix a bug in argument passing. When the expression is already a reference, we don't need or want to create a reference to the reference to pass the original reference. Instead just pass the reference itself.

add FIXME for POINTER and ALLOCATABLE

tighten some checks
fix type mismatch on assignment
misc fixes for LAPACK
format bullet items
Share pgmath description in lowering and folding. Simplify intrinsic lowering.

Also add more intrinsics lowering:
- ICHAR, ACHAR and plug all pgmath.

Fix libpgmath linking regression after D78215

Rename isNotInMemory to needToMaterialize. Fix StaticMultimapView usage for clang
…lex of one precision to a complex of another.

add a test.
untabify

Add a test and fix some bugs for F77 array lowering

More work on LAPACK

LAPACK: changes to get xerbla_array.f to compile clean.

Remove the special case handling of LOGICAL and i1.
Implement subroutine calls with alternate returns.
…isions.

move safe conversion to builder. rework FirOpBuilder for KindMapping.

fixes all the internal conversion problems when lowering intrinsics as exposed by BLAS.

fixes logical conversions
add back a change lost in merge

Start converting the speculative conversion of logical to bool to use lazy type coercion. Fix bugs that fall out of that conversion.

fix compilation failure. function was merged twice.

small cleanup on comments, missing cast
remove stray change

Wrap a CHARACTER literal in a box.
using the local tools setup.

alternative fix. this one works on the Mac as well.

Move the lib files around so they align with the include directory.  Rework the cmake files and use the new styles there.

Moving some code around. Renaming files to be more reflective of what they do currently. Refactoring comingled code into separate files.
* alternate return test
…ing calls. More work is needed here.

review suggestions

Remove the positive step check. Fortran has decrementing loops.

fix typo

update the LAPACK issues list

Dominance.h moved

fix breakage in tests
LEN_TRIM inline implementation + make character length index type

- Use fir::iter_while operation to provide a scalar implementation
of LEN_TRIM that is inlined. It is just a loop that goes from
the right most character to the first one and stop as soon as
a character is not a blank.
- Change getLengthType to return the indexType. It makes loop
manipulation of character easier. A few functions that expected
i64 for the length are modified to support whatever integer type
getLengthType is returning.

Conflicts:
	flang/lib/Lower/FIRBuilder.cpp

Conflicts:
	flang/LAPACK-bugs.txt
…really fix the lowering of procedure calls and their types.
To improve build times, lop off the f18 tool's dependence on Lowering. Moved the functionality to bbc, added some option support, and am able to run 3 of the 4 tests. Marked the last as expected fail.

Conflicts:
	flang/tools/f18/CMakeLists.txt

Remove raw_ostream references from Decimal
move towards making conversions more formal in the bridge

Removes some old aliases. Use the long names.

rename function
* I/O condition specifier branching

* review update
schweitzpgi and others added 18 commits July 11, 2020 13:14
Follow convention to capitalize Fortran_main.c
This patch tries to gather the actual requirements of what is expected
for upcoming patches.
[flang][OpenMP] downstream merge after 3d0b760
…oduces

some new types and operations that distinguish the runtime properties of
array shape and array slicing operations. Additionally, we refine the
array_coor and embox operations to use these new values.

fix bug in pretty printer. add test.

review comment - fix assert text

add an assert to catch cases when shape is missing, etc.
Refine the IR as it relates to shapes and slices of arrays. This intr…
[Porting] Port a sample test in not-test/ to test/Fir.
…tation. Theextension is not presently used. However, the plan will be to experiment and see if it helps simplify lowering.
[flang] Adopt NoRegionArguments (WhereOp) and ParentOneOf (ResultOp) traits
Extend !fir.char to improve disambiguation of CHARACTER type
This patch adds lowering support for OpenMP-4.5 taskwait construct to
OpenMP Dialect operations.
Copy link
Collaborator

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me.

Some nits inline.


program taskwait

integer :: a,b,c
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Is this the usual alignment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention here, is to have some code/operation at least. This helps in seeing the actual lowering operation placed in correct place(However this test doesn't bother/check's what's above and below).

!$OMP TASKWAIT
!FIRDialect: omp.taskwait
!LLVMIRDialect: omp.taskwait
!LLVMIR: %{{.*}} = call i32 @__kmpc_omp_taskwait(%struct.ident_t* @1, i32 %omp_global_thread_num)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to be less specific about variable names (@1, omp_global_thread_num) here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, Thanks for pointing this out.
I'll do it barrier test too. :)

@@ -41,4 +41,19 @@ TEST_F(OpenMPLoweringTest, Barrier) {
EXPECT_EQ(succeeded(barrierOp.verify()), true);
}

TEST_F(OpenMPLoweringTest, TaskWait) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to share some code between these tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be done(by clustering directives in one test and check) but IMO that will defeat the purpose of unit test. right ?
The common code that can be shared across all is already in fixture.

Copy link
Collaborator

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @SouraVX. LGTM.

@SouraVX
Copy link
Collaborator Author

SouraVX commented Jul 16, 2020

Thanks for reviewing this :). I'm raising review on Phabricator, so that community can also have a look. Share concerns if any!

Copy link

@schweitzpgi schweitzpgi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just let me know when you want to merge this.

@SouraVX
Copy link
Collaborator Author

SouraVX commented Jul 17, 2020

Please go ahead and merge it. I'll push in the phab review soon.
Thanks!

@schweitzpgi
Copy link

Merged.

SouraVX added a commit to llvm/llvm-project that referenced this pull request Jul 17, 2020
Summary:
This patch lower `!OMP TASKWAIT` construct from PFT to
OpenMPDialect operations.
Construct is lowered with conformance to OpenMP 4.5 spec.

Patch is carved out of following approved PR:
flang-compiler#280

Reviewed By: kiranchandramohan, clementval

Differential Revision: https://reviews.llvm.org/D83983
arichardson pushed a commit to arichardson/llvm-project that referenced this pull request Mar 21, 2021
Summary:
This patch lower `!OMP TASKWAIT` construct from PFT to
OpenMPDialect operations.
Construct is lowered with conformance to OpenMP 4.5 spec.

Patch is carved out of following approved PR:
flang-compiler/f18-llvm-project#280

Reviewed By: kiranchandramohan, clementval

Differential Revision: https://reviews.llvm.org/D83983
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this pull request Oct 7, 2022
Summary:
This patch lower `!OMP TASKWAIT` construct from PFT to
OpenMPDialect operations.
Construct is lowered with conformance to OpenMP 4.5 spec.

Patch is carved out of following approved PR:
flang-compiler/f18-llvm-project#280

Reviewed By: kiranchandramohan, clementval

Differential Revision: https://reviews.llvm.org/D83983
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet