[flang][openacc] use location of end directive for exit operations

[akuhlens]

Make sure to preserve the location of the end statement on data declarations for use in debugging OpenACC runtime.

[llvmbot]

@llvm/pr-subscribers-openacc

@llvm/pr-subscribers-flang-fir-hlfir

Author: Andre Kuhlenschmidt (akuhlens)

Changes

Make sure to preserve the location of the end statement on data declarations for use in debugging OpenACC runtime.

---

Full diff: https://github.com/llvm/llvm-project/pull/140763.diff

1 Files Affected:

• (modified) flang/lib/Lower/OpenACC.cpp (+21-16)

diff --git a/flang/lib/Lower/OpenACC.cpp b/flang/lib/Lower/OpenACC.cpp
index e1918288d6de3..3ff11dfaab311 100644
--- a/flang/lib/Lower/OpenACC.cpp
+++ b/flang/lib/Lower/OpenACC.cpp
@@ -810,23 +810,25 @@ static void genDeclareDataOperandOperationsWithModifier(
 }
 
 template <typename EntryOp, typename ExitOp>
-static void genDataExitOperations(fir::FirOpBuilder &builder,
-                                  llvm::SmallVector<mlir::Value> operands,
-                                  bool structured) {
+static void
+genDataExitOperations(fir::FirOpBuilder &builder,
+                      llvm::SmallVector<mlir::Value> operands, bool structured,
+                      std::optional<mlir::Location> exitLoc = std::nullopt) {
   for (mlir::Value operand : operands) {
     auto entryOp = mlir::dyn_cast_or_null<EntryOp>(operand.getDefiningOp());
     assert(entryOp && "data entry op expected");
+    auto opLoc = exitLoc ? *exitLoc : entryOp.getLoc();
     if constexpr (std::is_same_v<ExitOp, mlir::acc::CopyoutOp> ||
                   std::is_same_v<ExitOp, mlir::acc::UpdateHostOp>)
       builder.create<ExitOp>(
-          entryOp.getLoc(), entryOp.getAccVar(), entryOp.getVar(),
-          entryOp.getVarType(), entryOp.getBounds(), entryOp.getAsyncOperands(),
+          opLoc, entryOp.getAccVar(), entryOp.getVar(), entryOp.getVarType(),
+          entryOp.getBounds(), entryOp.getAsyncOperands(),
           entryOp.getAsyncOperandsDeviceTypeAttr(), entryOp.getAsyncOnlyAttr(),
           entryOp.getDataClause(), structured, entryOp.getImplicit(),
           builder.getStringAttr(*entryOp.getName()));
     else
       builder.create<ExitOp>(
-          entryOp.getLoc(), entryOp.getAccVar(), entryOp.getBounds(),
+          opLoc, entryOp.getAccVar(), entryOp.getBounds(),
           entryOp.getAsyncOperands(), entryOp.getAsyncOperandsDeviceTypeAttr(),
           entryOp.getAsyncOnlyAttr(), entryOp.getDataClause(), structured,
           entryOp.getImplicit(), builder.getStringAttr(*entryOp.getName()));
@@ -3017,6 +3019,7 @@ static Op createComputeOp(
 
 static void genACCDataOp(Fortran::lower::AbstractConverter &converter,
                          mlir::Location currentLocation,
+                         mlir::Location endLocation,
                          Fortran::lower::pft::Evaluation &eval,
                          Fortran::semantics::SemanticsContext &semanticsContext,
                          Fortran::lower::StatementContext &stmtCtx,
@@ -3211,19 +3214,19 @@ static void genACCDataOp(Fortran::lower::AbstractConverter &converter,
 
   // Create the exit operations after the region.
   genDataExitOperations<mlir::acc::CopyinOp, mlir::acc::CopyoutOp>(
-      builder, copyEntryOperands, /*structured=*/true);
+      builder, copyEntryOperands, /*structured=*/true, endLocation);
   genDataExitOperations<mlir::acc::CopyinOp, mlir::acc::DeleteOp>(
-      builder, copyinEntryOperands, /*structured=*/true);
+      builder, copyinEntryOperands, /*structured=*/true, endLocation);
   genDataExitOperations<mlir::acc::CreateOp, mlir::acc::CopyoutOp>(
-      builder, copyoutEntryOperands, /*structured=*/true);
+      builder, copyoutEntryOperands, /*structured=*/true, endLocation);
   genDataExitOperations<mlir::acc::AttachOp, mlir::acc::DetachOp>(
-      builder, attachEntryOperands, /*structured=*/true);
+      builder, attachEntryOperands, /*structured=*/true, endLocation);
   genDataExitOperations<mlir::acc::CreateOp, mlir::acc::DeleteOp>(
-      builder, createEntryOperands, /*structured=*/true);
+      builder, createEntryOperands, /*structured=*/true, endLocation);
   genDataExitOperations<mlir::acc::NoCreateOp, mlir::acc::DeleteOp>(
-      builder, nocreateEntryOperands, /*structured=*/true);
+      builder, nocreateEntryOperands, /*structured=*/true, endLocation);
   genDataExitOperations<mlir::acc::PresentOp, mlir::acc::DeleteOp>(
-      builder, presentEntryOperands, /*structured=*/true);
+      builder, presentEntryOperands, /*structured=*/true, endLocation);
 
   builder.restoreInsertionPoint(insPt);
 }
@@ -3300,7 +3303,9 @@ genACC(Fortran::lower::AbstractConverter &converter,
       std::get<Fortran::parser::AccBlockDirective>(beginBlockDirective.t);
   const auto &accClauseList =
       std::get<Fortran::parser::AccClauseList>(beginBlockDirective.t);
-
+  const auto &endBlockDirective =
+      std::get<Fortran::parser::AccEndBlockDirective>(blockConstruct.t);
+  mlir::Location endLocation = converter.genLocation(endBlockDirective.source);
   mlir::Location currentLocation = converter.genLocation(blockDirective.source);
   Fortran::lower::StatementContext stmtCtx;
 
@@ -3309,8 +3314,8 @@ genACC(Fortran::lower::AbstractConverter &converter,
                                            semanticsContext, stmtCtx,
                                            accClauseList);
   } else if (blockDirective.v == llvm::acc::ACCD_data) {
-    genACCDataOp(converter, currentLocation, eval, semanticsContext, stmtCtx,
-                 accClauseList);
+    genACCDataOp(converter, currentLocation, endLocation, eval,
+                 semanticsContext, stmtCtx, accClauseList);
   } else if (blockDirective.v == llvm::acc::ACCD_serial) {
     createComputeOp<mlir::acc::SerialOp>(converter, currentLocation, eval,
                                          semanticsContext, stmtCtx,

[akuhlens]

I didn't see a good way of testing this change. I am open to suggestions if you think it needs a test. I have checked manually that the correct location information ends up at the exit operation calls.

[clementval]

Can you add a test in flang/test/Lower/OpenACC/locations.f90?

[clementval]

LGTM! Thanks for addressing my comments.

[razvanlupusoru]

Nice work! I am glad that the location information for the end was available - I was worried it wasn't!

[razvanlupusoru]

Thank you! Nice work!