[Mlir-commits] [mlir] [Flang][HLFIR][OpenMP] Fix offloading tests broken by HLFIR (PR #69457)

Sergio Afonso llvmlistbot at llvm.org
Fri Oct 20 04:10:44 PDT 2023


https://github.com/skatrak updated https://github.com/llvm/llvm-project/pull/69457

>From ecacb0d38fc50de5d27c978c1959e467254b2a44 Mon Sep 17 00:00:00 2001
From: Sergio Afonso <safonsof at amd.com>
Date: Wed, 18 Oct 2023 12:25:41 +0100
Subject: [PATCH 1/4] [Flang][HLFIR][OpenMP] Fix offloading tests broken by
 HLFIR

This patch makes changes to the early outlining pass to avoid compiler crashes
due to not handling `hlfir.declare` operations correctly. That pass is intended
to eventually be removed (#67319), but in the meantime this fixes some issues
arising in different parts of the OpenMP offloading compilation process.

The main changes included in this patch are the following:
  - Added support for mapped values defined by an `hlfir.declare` operation.
  These operations are now kept in outlined target functions, so that both of
  their outputs (base and original base) are available to the corresponding
  `omp.target`'s map arguments and region.
  - Added a fix by @agozillon to prevent unused map clauses from producing a
  compiler crash. All these unused mapped variables are added to the outlined
  function's inputs.
  - Added a fix to the OpenMP translation to MLIR to support integer arguments
  to these outlined functions. This enables successfully compiling and running
  the tests in opemp/libomptarget/test/offloading/fortran using HLFIR.

Co-authored-by: agozillon <Andrew.Gozillon at amd.com>
---
 .../Transforms/OMPEarlyOutlining.cpp          | 41 +++++++++++++++++--
 flang/test/Lower/OpenMP/FIR/array-bounds.f90  |  2 +-
 .../declare-target-implicit-tarop-cap.f90     |  8 ++--
 .../OpenMP/{FIR => }/function-filtering-2.f90 | 12 +++---
 .../OpenMP/{FIR => }/function-filtering.f90   | 12 +++---
 5 files changed, 55 insertions(+), 20 deletions(-)
 rename flang/test/Lower/OpenMP/{FIR => }/declare-target-implicit-tarop-cap.f90 (87%)
 rename flang/test/Lower/OpenMP/{FIR => }/function-filtering-2.f90 (73%)
 rename flang/test/Lower/OpenMP/{FIR => }/function-filtering.f90 (66%)

diff --git a/flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp b/flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp
index 355a687d5f88588..436472459280e32 100644
--- a/flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp
+++ b/flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp
@@ -1,6 +1,7 @@
 #include "flang/Optimizer/Dialect/FIRDialect.h"
 #include "flang/Optimizer/Dialect/FIROps.h"
 #include "flang/Optimizer/Dialect/FIRType.h"
+#include "flang/Optimizer/HLFIR/HLFIROps.h"
 #include "flang/Optimizer/Support/InternalNames.h"
 #include "flang/Optimizer/Transforms/Passes.h"
 #include "mlir/Dialect/Func/IR/FuncOps.h"
@@ -99,6 +100,18 @@ class OMPEarlyOutliningPass
       return;
     }
 
+    if (auto declareOp = mlir::dyn_cast_if_present<hlfir::DeclareOp>(
+            varPtr.getDefiningOp())) {
+      auto clone = llvm::cast<hlfir::DeclareOp>(
+          cloneArgAndChildren(builder, declareOp, inputs, newInputs));
+      mlir::Value newBase = clone.getBase();
+      mlir::Value newOrigBase = clone.getOriginalBase();
+      mapInfoMap.map(varPtr, newOrigBase);
+      valueMap.map(declareOp.getBase(), newBase);
+      valueMap.map(declareOp.getOriginalBase(), newOrigBase);
+      return;
+    }
+
     if (isAddressOfGlobalDeclareTarget(varPtr)) {
       fir::AddrOfOp addrOp =
           mlir::dyn_cast<fir::AddrOfOp>(varPtr.getDefiningOp());
@@ -127,19 +140,41 @@ class OMPEarlyOutliningPass
     llvm::SetVector<mlir::Value> inputs;
     mlir::Region &targetRegion = targetOp.getRegion();
     mlir::getUsedValuesDefinedAbove(targetRegion, inputs);
-    
-    // filter out declareTarget and map entries which are specially handled
+
+    // Collect all map info. Even non-used maps must be collected to avoid ICEs.
+    for (mlir::Value oper : targetOp->getOperands()) {
+      if (auto mapEntry =
+              mlir::dyn_cast<mlir::omp::MapInfoOp>(oper.getDefiningOp())) {
+        if (!inputs.contains(mapEntry.getVarPtr()))
+          inputs.insert(mapEntry.getVarPtr());
+      }
+    }
+
+    // Filter out declare-target and map entries which are specially handled
     // at the moment, so we do not wish these to end up as function arguments
     // which would just be more noise in the IR.
+    llvm::SmallVector<mlir::Value> blockArgs;
     for (llvm::SetVector<mlir::Value>::iterator iter = inputs.begin(); iter != inputs.end();) {
       if (mlir::isa_and_nonnull<mlir::omp::MapInfoOp>(iter->getDefiningOp()) ||
           isAddressOfGlobalDeclareTarget(*iter)) {
         iter = inputs.erase(iter);
+      } else if (auto declareOp = mlir::dyn_cast_if_present<hlfir::DeclareOp>(
+                     iter->getDefiningOp())) {
+        if (!declareOp.getMemref().getDefiningOp())
+          blockArgs.push_back(declareOp.getMemref());
+        iter = inputs.erase(iter);
       } else {
         ++iter;
       }
     }
 
+    // Add function arguments to the list of inputs if they are used by an
+    // hlfir.declare operation.
+    for (mlir::Value arg : blockArgs) {
+      if (!inputs.contains(arg))
+        inputs.insert(arg);
+    }
+
     // Create new function and initialize
     mlir::FunctionType funcType = builder.getFunctionType(
         mlir::TypeRange(inputs.getArrayRef()), mlir::TypeRange());
@@ -218,7 +253,7 @@ class OMPEarlyOutliningPass
     return newFunc;
   }
 
-  // Returns true if a target region was found int the function.
+  // Returns true if a target region was found in the function.
   bool outlineTargetOps(mlir::OpBuilder &builder,
                         mlir::func::FuncOp &functionOp,
                         mlir::ModuleOp &moduleOp,
diff --git a/flang/test/Lower/OpenMP/FIR/array-bounds.f90 b/flang/test/Lower/OpenMP/FIR/array-bounds.f90
index 697ff44176b3f10..748257f8dcc3024 100644
--- a/flang/test/Lower/OpenMP/FIR/array-bounds.f90
+++ b/flang/test/Lower/OpenMP/FIR/array-bounds.f90
@@ -48,7 +48,7 @@ end subroutine read_write_section
 module assumed_array_routines
     contains
 !DEVICE-LABEL: func.func @_QMassumed_array_routinesPassumed_shape_array_omp_outline_0(
-!DEVICE-SAME: %[[ARG0:.*]]: !fir.ref<i32>, %[[ARG1:.*]]: !fir.box<!fir.array<?xi32>>) attributes {omp.declare_target = #omp.declaretarget<device_type = (host), capture_clause = (to)>, omp.outline_parent_name = "_QMassumed_array_routinesPassumed_shape_array"} {
+!DEVICE-SAME: %[[ARG0:.*]]: !fir.ref<i32>, %[[ARG1:.*]]: !fir.box<!fir.array<?xi32>>, %[[ARG2:.*]]: !fir.ref<!fir.array<?xi32>>) attributes {omp.declare_target = #omp.declaretarget<device_type = (host), capture_clause = (to)>, omp.outline_parent_name = "_QMassumed_array_routinesPassumed_shape_array"} {
 !DEVICE: %[[C0:.*]] = arith.constant 1 : index
 !DEVICE: %[[C1:.*]] = arith.constant 4 : index
 !DEVICE: %[[C2:.*]] = arith.constant 0 : index
diff --git a/flang/test/Lower/OpenMP/FIR/declare-target-implicit-tarop-cap.f90 b/flang/test/Lower/OpenMP/declare-target-implicit-tarop-cap.f90
similarity index 87%
rename from flang/test/Lower/OpenMP/FIR/declare-target-implicit-tarop-cap.f90
rename to flang/test/Lower/OpenMP/declare-target-implicit-tarop-cap.f90
index cee24883f47151b..0f548600f760178 100644
--- a/flang/test/Lower/OpenMP/FIR/declare-target-implicit-tarop-cap.f90
+++ b/flang/test/Lower/OpenMP/declare-target-implicit-tarop-cap.f90
@@ -1,7 +1,7 @@
-!RUN: %flang_fc1 -emit-fir -fopenmp %s -o - | FileCheck %s
-!RUN: %flang_fc1 -emit-fir -fopenmp -fopenmp-is-device %s -o - | FileCheck %s  --check-prefix=DEVICE
-!RUN: bbc -emit-fir -fopenmp %s -o - | FileCheck %s
-!RUN: bbc -emit-fir -fopenmp -fopenmp-is-target-device %s -o - | FileCheck %s --check-prefix=DEVICE
+!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+!RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-is-device %s -o - | FileCheck %s  --check-prefix=DEVICE
+!RUN: bbc -emit-hlfir -fopenmp %s -o - | FileCheck %s
+!RUN: bbc -emit-hlfir -fopenmp -fopenmp-is-target-device %s -o - | FileCheck %s --check-prefix=DEVICE
 
 ! DEVICE-LABEL: func.func @_QPimplicit_capture
 ! DEVICE-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (nohost), capture_clause = (to)>{{.*}}}
diff --git a/flang/test/Lower/OpenMP/FIR/function-filtering-2.f90 b/flang/test/Lower/OpenMP/function-filtering-2.f90
similarity index 73%
rename from flang/test/Lower/OpenMP/FIR/function-filtering-2.f90
rename to flang/test/Lower/OpenMP/function-filtering-2.f90
index 8065467504dd27d..8219be5ad1e40ca 100644
--- a/flang/test/Lower/OpenMP/FIR/function-filtering-2.f90
+++ b/flang/test/Lower/OpenMP/function-filtering-2.f90
@@ -1,9 +1,9 @@
-! RUN: %flang_fc1 -fopenmp -emit-llvm %s -o - | FileCheck --check-prefix=LLVM %s
-! RUN: %flang_fc1 -fopenmp -emit-mlir %s -o - | FileCheck --check-prefix=MLIR %s
-! RUN: %flang_fc1 -fopenmp -fopenmp-is-target-device -emit-llvm %s -o - | FileCheck --check-prefix=LLVM %s
-! RUN: %flang_fc1 -fopenmp -fopenmp-is-target-device -emit-mlir %s -o - | FileCheck --check-prefix=MLIR %s
-! RUN: bbc -fopenmp -emit-fir %s -o - | FileCheck --check-prefixes=MLIR-HOST,MLIR-ALL %s
-! RUN: bbc -fopenmp -fopenmp-is-target-device -emit-fir %s -o - | FileCheck --check-prefixes=MLIR-DEVICE,MLIR-ALL %s
+! RUN: %flang_fc1 -fopenmp -flang-experimental-hlfir -emit-llvm %s -o - | FileCheck --check-prefix=LLVM %s
+! RUN: %flang_fc1 -fopenmp -emit-hlfir %s -o - | FileCheck --check-prefix=MLIR %s
+! RUN: %flang_fc1 -fopenmp -fopenmp-is-target-device -flang-experimental-hlfir -emit-llvm %s -o - | FileCheck --check-prefix=LLVM %s
+! RUN: %flang_fc1 -fopenmp -fopenmp-is-target-device -emit-hlfir %s -o - | FileCheck --check-prefix=MLIR %s
+! RUN: bbc -fopenmp -emit-hlfir %s -o - | FileCheck --check-prefixes=MLIR-HOST,MLIR-ALL %s
+! RUN: bbc -fopenmp -fopenmp-is-target-device -emit-hlfir %s -o - | FileCheck --check-prefixes=MLIR-DEVICE,MLIR-ALL %s
 
 ! MLIR: func.func @{{.*}}implicit_invocation() attributes {omp.declare_target = #omp.declaretarget<device_type = (nohost), capture_clause = (to)>}
 ! MLIR: return
diff --git a/flang/test/Lower/OpenMP/FIR/function-filtering.f90 b/flang/test/Lower/OpenMP/function-filtering.f90
similarity index 66%
rename from flang/test/Lower/OpenMP/FIR/function-filtering.f90
rename to flang/test/Lower/OpenMP/function-filtering.f90
index c0ca351b9b335f0..3de14aa4709fc4c 100644
--- a/flang/test/Lower/OpenMP/FIR/function-filtering.f90
+++ b/flang/test/Lower/OpenMP/function-filtering.f90
@@ -1,9 +1,9 @@
-! RUN: %flang_fc1 -fopenmp -emit-llvm %s -o - | FileCheck --check-prefixes=LLVM-HOST,LLVM-ALL %s
-! RUN: %flang_fc1 -fopenmp -emit-mlir %s -o - | FileCheck --check-prefixes=MLIR-HOST,MLIR-ALL %s
-! RUN: %flang_fc1 -fopenmp -fopenmp-is-target-device -emit-llvm %s -o - | FileCheck --check-prefixes=LLVM-DEVICE,LLVM-ALL %s
-! RUN: %flang_fc1 -fopenmp -fopenmp-is-target-device -emit-mlir %s -o - | FileCheck --check-prefixes=MLIR-DEVICE,MLIR-ALL %s
-! RUN: bbc -fopenmp -emit-fir %s -o - | FileCheck --check-prefixes=MLIR-HOST,MLIR-ALL %s
-! RUN: bbc -fopenmp -fopenmp-is-target-device -emit-fir %s -o - | FileCheck --check-prefixes=MLIR-DEVICE,MLIR-ALL %s
+! RUN: %flang_fc1 -fopenmp -flang-experimental-hlfir -emit-llvm %s -o - | FileCheck --check-prefixes=LLVM-HOST,LLVM-ALL %s
+! RUN: %flang_fc1 -fopenmp -emit-hlfir %s -o - | FileCheck --check-prefixes=MLIR-HOST,MLIR-ALL %s
+! RUN: %flang_fc1 -fopenmp -fopenmp-is-target-device -flang-experimental-hlfir -emit-llvm %s -o - | FileCheck --check-prefixes=LLVM-DEVICE,LLVM-ALL %s
+! RUN: %flang_fc1 -fopenmp -fopenmp-is-target-device -emit-hlfir %s -o - | FileCheck --check-prefixes=MLIR-DEVICE,MLIR-ALL %s
+! RUN: bbc -fopenmp -emit-hlfir %s -o - | FileCheck --check-prefixes=MLIR-HOST,MLIR-ALL %s
+! RUN: bbc -fopenmp -fopenmp-is-target-device -emit-hlfir %s -o - | FileCheck --check-prefixes=MLIR-DEVICE,MLIR-ALL %s
 
 ! Check that the correct LLVM IR functions are kept for the host and device
 ! after running the whole set of translation and transformation passes from

>From d3e31ab801f4cc47fff6fedb0c22c488cc221adb Mon Sep 17 00:00:00 2001
From: Sergio Afonso <safonsof at amd.com>
Date: Fri, 20 Oct 2023 11:41:38 +0100
Subject: [PATCH 2/4] Fix OpenMP dialect translation to LLVM IR to support
 integer parameters

---
 .../LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp   | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index e3dc68a1b8b7d72..447d327a0000c7e 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -2042,13 +2042,13 @@ createDeviceArgumentAccessor(llvm::Argument &arg, llvm::Value *input,
                                : llvm::Type::getInt64Ty(builder.getContext()),
                            ompBuilder.M.getDataLayout().getAllocaAddrSpace());
   llvm::Value *addrAscast =
-      builder.CreatePointerBitCastOrAddrSpaceCast(addr, input->getType());
-  builder.CreateStore(&arg, addrAscast);
+      arg.getType()->isPointerTy()
+          ? builder.CreatePointerBitCastOrAddrSpaceCast(addr, input->getType())
+          : addr;
 
+  builder.CreateStore(&arg, addrAscast);
   builder.restoreIP(codeGenIP);
-
   retVal = builder.CreateLoad(arg.getType(), addrAscast);
-
   return builder.saveIP();
 }
 

>From 532e1683a1c33c71fd3454418c3437f6e87206c7 Mon Sep 17 00:00:00 2001
From: Sergio Afonso <safonsof at amd.com>
Date: Fri, 20 Oct 2023 11:48:23 +0100
Subject: [PATCH 3/4] Complete list of hlfir.declare args to be passed as
 function inputs

---
 flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp b/flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp
index 436472459280e32..c2bc9903b844cbc 100644
--- a/flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp
+++ b/flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp
@@ -160,8 +160,13 @@ class OMPEarlyOutliningPass
         iter = inputs.erase(iter);
       } else if (auto declareOp = mlir::dyn_cast_if_present<hlfir::DeclareOp>(
                      iter->getDefiningOp())) {
-        if (!declareOp.getMemref().getDefiningOp())
-          blockArgs.push_back(declareOp.getMemref());
+        // Gather hlfir.declare arguments to be added later, after the
+        // hlfir.declare operation itself has been removed as an input.
+        blockArgs.push_back(declareOp.getMemref());
+        if (mlir::Value shape = declareOp.getShape())
+          blockArgs.push_back(shape);
+        for (mlir::Value typeParam : declareOp.getTypeparams())
+          blockArgs.push_back(typeParam);
         iter = inputs.erase(iter);
       } else {
         ++iter;
@@ -171,7 +176,7 @@ class OMPEarlyOutliningPass
     // Add function arguments to the list of inputs if they are used by an
     // hlfir.declare operation.
     for (mlir::Value arg : blockArgs) {
-      if (!inputs.contains(arg))
+      if (!arg.getDefiningOp() && !inputs.contains(arg))
         inputs.insert(arg);
     }
 

>From b5dca9f6fe99738360d2b7ba71e81166c0d38c62 Mon Sep 17 00:00:00 2001
From: Sergio Afonso <safonsof at amd.com>
Date: Fri, 20 Oct 2023 12:10:11 +0100
Subject: [PATCH 4/4] Add comment as suggested by reviewer

---
 flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp b/flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp
index c2bc9903b844cbc..92fbdd0bbf5d4a1 100644
--- a/flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp
+++ b/flang/lib/Optimizer/Transforms/OMPEarlyOutlining.cpp
@@ -100,6 +100,8 @@ class OMPEarlyOutliningPass
       return;
     }
 
+    // Clone into the outlined function all hlfir.declare ops that define inputs
+    // to the target region and set up remapping of its inputs and outputs.
     if (auto declareOp = mlir::dyn_cast_if_present<hlfir::DeclareOp>(
             varPtr.getDefiningOp())) {
       auto clone = llvm::cast<hlfir::DeclareOp>(



More information about the Mlir-commits mailing list