This is an archive of the discontinued LLVM Phabricator instance.

[mlir][openacc] Add separate acc data operations for OpenACC data clauses
ClosedPublic

Authored by razvanlupusoru on Apr 14 2023, 3:33 PM.

Details

Summary

As outlined in [1], data clauses are now implemented as separate operations
from the constructs that they belong to. Some of the highlighted benefits:

  • Correctly represent dataflow of data operations
  • Easier to track debugging information
  • Friendlier to add attributes and to optimize operations

For now, all of the other operand lists are being kept until all references
to them in LLVM can be removed (such as those in flang lowering)

[1] https://discourse.llvm.org/t/rfc-openacc-dialect-data-operation-improvements/69825

Diff Detail

Event Timeline

razvanlupusoru created this revision.Apr 14 2023, 3:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2023, 3:33 PM
razvanlupusoru requested review of this revision.Apr 14 2023, 3:33 PM

Added memory effects to recently added acc.kernels and acc.serial.
Added newlines at end of newly added files.

I just did a quick overview and I have some questions and nits.

mlir/include/mlir/Dialect/OpenACC/CMakeLists.txt
27

Add new line here and couple of other files.

mlir/include/mlir/Dialect/OpenACC/OpenACCBase.td
2

Is this change necessary for this patch to work?

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
1

I think the line is too long now.

333

Should this be OpenACC_PointerLikeTypeInterface for the type? Same comment for other dataClauseOperands

mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
207–208

Unrelated change

razvanlupusoru marked an inline comment as done.Apr 14 2023, 4:20 PM
razvanlupusoru added inline comments.
mlir/include/mlir/Dialect/OpenACC/OpenACCBase.td
2

I need to check this. I include this file in both OpenACCOps.td and OpenACCOpsTypes.td. I would need to check whether the latter works without a dialect definition.

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
1

Will fix.

333

Yes. Nice catch.

mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
207–208

OK. I will restore this.

vzakhari added inline comments.Apr 14 2023, 5:39 PM
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
143

It would be useful to describe the order in which bounds are listed. Is it matching the language order?

You probably noticed it but there are couple of failures on the pre-commit ci on the flang side

Flang :: Lower/OpenACC/acc-data-operands.f90
Flang :: Lower/OpenACC/acc-data.f90
Flang :: Lower/OpenACC/acc-enter-data.f90
Flang :: Lower/OpenACC/acc-exit-data.f90
Flang :: Lower/OpenACC/acc-parallel-loop.f90
Flang :: Lower/OpenACC/acc-parallel.f90
Flang :: Lower/OpenACC/acc-serial-loop.f90
Flang :: Lower/OpenACC/acc-serial.f90
Flang :: Lower/OpenACC/locations.f90
razvanlupusoru added inline comments.Apr 17 2023, 8:52 AM
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
143

Thank you for catching this ambiguity. I added the following wording:
// The bounds are represented in rank order. Rank 0 (innermost dimension) is the first.

Making the bounds match language order would cause ambiguity since we would have to know the dialect (and its source language) that the acc dialect is being paired with.

The choice of representing bounds in rank order with rank-0 being first also matches other dialects like vector dialect:
https://mlir.llvm.org/docs/Dialects/Vector/#alternatives-for-lowering-an-n-d-vector-type-to-llvm
"Consider a vector of rank n with static sizes {s_0, ... s_{n-1}} (i.e. an MLIR vector<s_0x...s_{n-1}xf32>)"

razvanlupusoru added inline comments.Apr 17 2023, 9:07 AM
mlir/include/mlir/Dialect/OpenACC/OpenACCBase.td
2

I confirm that this change is needed due to the fact that the OpenACCOpsTypes.td file also needs a dialect definition to define a new type:

class OpenACC_Type<string name, string typeMnemonic> : TypeDef<OpenACC_Dialect, name> {
vzakhari added inline comments.Apr 17 2023, 9:13 AM
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
143

Thank you, Razvan! It looks great!

clementval added inline comments.Apr 17 2023, 10:17 AM
mlir/include/mlir/Dialect/OpenACC/OpenACCBase.td
2

Ok makes sense.

clementval added inline comments.Apr 17 2023, 11:09 AM
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
89–99

Shouldn't we switch the naming here. It's DataClase enum but we have CopyinOp and so on. I would rename the enum attr from Op to Clause and the actual operation defined below from Clause to Op.

now

def OpenACC_PresentClause : OpenACC_DataEntryClause<"present",
    "mlir::acc::DataClause::acc_present"> {
  let summary = "Specifies that the variable is already present on device.";
}

after

def OpenACC_PresentOp : OpenACC_DataEntryOp<"present",
    "mlir::acc::DataClause::acc_present"> {
  let summary = "Specifies that the variable is already present on device.";
}
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
89–99

Agreed.

Addressed reviewer comments including:

  • Swapping clause and op naming so that MLIR operations end with Op.
  • Clarifying rank ordering for bounds.
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2023, 11:56 AM
razvanlupusoru marked 8 inline comments as done.Apr 17 2023, 11:58 AM
clementval added inline comments.Apr 17 2023, 12:48 PM
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
89–99

Thanks for the update. I think it reads better now.

192

This comment needs an update.

206

This comment needs an update.

277

This comment needs an update.

Fixed incorrect comment for acc clause modifier getters.

razvanlupusoru marked 3 inline comments as done.Apr 17 2023, 1:34 PM
This revision is now accepted and ready to land.Apr 17 2023, 1:35 PM
vzakhari accepted this revision.Apr 17 2023, 2:46 PM

Removed MemoryEffects interface from data operations. I had a comment that the current modeling was not ideal since there are host, device, and runtime effects. The effects also differ depending on whether the acc target is a shared memory system (as defined in section 2.6). Since more design is needed for modeling, I just removed them from the data operations.

clementval accepted this revision.Apr 18 2023, 3:24 PM

Still LGTM

Fix for buildbot failures after I landed this change:
https://reviews.llvm.org/D148673

mehdi_amini added inline comments.Apr 19 2023, 8:49 PM
mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
50

The usual way we have been handling this in-tree has been to use "external interfaces", see for example scf::registerBufferizableOpInterfaceExternalModels() and look into the implementation for registerAllDialects.

That way we keep dialects independents from each others!

razvanlupusoru added inline comments.Apr 20 2023, 4:10 PM
mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
50

Sounds like this is what I need to do here. I will investigate this suggestion and follow-up in the coming week. Thank you for your advice.