This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Flang][OpenMP] Implement lowering simd aligned to MLIR
AcceptedPublic

Authored by domada on Jan 27 2023, 7:03 AM.

Details

Summary

This patch implements lowering simd aligned clause from PFT to MLIR.

Generated MLIR code contains information which MLIR item needs to be aligned. If the user does not specify the alignment value then the default one is calculated on the basis of target features.

Diff Detail

Event Timeline

domada created this revision.Jan 27 2023, 7:03 AM
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
domada requested review of this revision.Jan 27 2023, 7:03 AM
kiranchandramohan requested changes to this revision.Feb 3 2023, 10:08 AM

For this patch, I would recommend moving the translation changes in MLIR into a separate patch.

Please see comments inline.

flang/lib/Lower/OpenMP.cpp
2438

Has the patch that added this function been reverted?

Is there are test for this case?

flang/test/Lower/OpenMP/simd.f90
168

We need to increase the test coverage.

171

From the standard:
Each list item must have C_PTR or Cray pointer type or have the POINTER or ALLOCATABLE attribute. Cray pointer support has been deprecated.

This array does not meet this criteria and hence not a good test. Will also be good to file a github issue to add this semantic check.

172–177

PARAM_ARG , ARG_0, ARG_1 etc do not seem to be used. Is there a reason for capturing?

mlir/test/Target/LLVMIR/openmp-llvm.mlir
762–777 ↗(On Diff #492742)

Can you minimise this test to what is only needed to check the translation of this clause?

Could you clarify what types are covered and what types are not covered by this change?

This revision now requires changes to proceed.Feb 3 2023, 10:08 AM
domada added inline comments.Mar 10 2023, 1:08 AM
flang/lib/Lower/OpenMP.cpp
2438

The patch has been restored: https://reviews.llvm.org/rGbaca3c150733c89686287ba4927c351eec9695e2

Default OpenMP simd alignment is tested here:
https://github.com/llvm/llvm-project/blob/main/clang/test/OpenMP/simd_metadata.c
Would you like to have Flang specific tests as well?

2443–2444

I think that we need to modify Flang parser so that the aligned item is modeled as Expr, not as a symbol.

IMO we should firstly modify parser and then update this review.

flang/test/Lower/OpenMP/simd.f90
171

I created github issue: https://github.com/llvm/llvm-project/issues/61161 to add this semantic check

domada planned changes to this revision.Apr 13 2023, 6:31 AM

Fortan parser needs to be modified before we will submit lowering simd aligned clause to MLIR.

flang/lib/Lower/OpenMP.cpp
2420–2421

OmpAlignedClause should be modified. It should contain list of Fortran::parser::Variable .

2442–2444

This loop should be modified. We should modify this loop so that it will generate the following MLIR code:

Input Fortran code:

integer, pointer :: a;
! ptr assignment etc.
!$omp simd aligned(a:256)

Pseudo MLIR code which need to be generated:

%a_ptr : !fir.ref<!fir.ref<!fir.ptr<i32>>
; ptr assignment etc.
%a_addr = load %a_ptr
 omp.simdloop aligned(%a_addr -> 256 : i64)

To do it we need to firstly refactor OmpAlignedClause so that it contains list of Fortran::parser::Variable. Then the for loop will look in the following way:

Fortran::lower::StatementContext stmtCtx;
for (const Fortran::parser::Variable &ompItem : ompvarList) {
  alignVars.push_back(fir::getBase(converter.genExprAddr(*Fortran::semantics::GetExpr(ompItem), stmtCtx));
  alignmentValues.push_back(alignmentValueAttr);
}
domada edited the summary of this revision. (Show Details)Jul 6 2023, 12:56 AM
domada updated this revision to Diff 537616.Jul 6 2023, 1:02 AM

Patch rebased. Use OmpObjectList instead of NameList. Added test cases for C_PTR and Fortran pointers.

It will be easier if you can split out the MLIR portions into a separate patch. From a quick look, that looks standalone.

domada updated this revision to Diff 548163.Aug 8 2023, 5:22 AM
domada retitled this revision from [Flang][OpenMP] Implement lowering simd aligned to LLVM IR to [MLIR][Flang][OpenMP] Implement lowering simd aligned to MLIR.
domada edited the summary of this revision. (Show Details)

Patch rebased.
Applied review remarks - scope of the patch is reduced. It contains only the changes which are required to generate MLIR code for simd aligned clause.

kiranchandramohan accepted this revision.Aug 9 2023, 7:29 AM

LG.

Please add couple more tests with:
-> More than one list item in the aligned clause.
-> List item that is a Fortran pointer.
Also check with the gfortran testsuite before you submit.

flang/lib/Lower/OpenMP.cpp
1199–1201

Nit: No braces.

1202

Nit: Expand auto.

1251

Would passing in a map to getTargetFeatures be better? Will that avoid a copy?

1262–1273

Nit: Can you use getOmpObjectSymbol here, given that Semantics will error out if it is a common block and we will not reach here?

1279

Nit: Does not talk about Less than 0 (which is the comparison used below).

This revision is now accepted and ready to land.Aug 9 2023, 7:29 AM