[MLIR][OpenMP] Changes to function-filtering pass #71850

TIFitis · 2023-11-09T19:13:29Z

Currently, when deleting the device functions in the second stage of filtering during MLIR to LLVM translation we can end up with invalid calls to these functions. This is because of the removal of the EarlyOutliningPass which would have otherwise gotten rid of any such calls.

This patch aims to alter the function filtering pass in the following way:
- Any host function is completely removed.
- Call to the host function are also removed and their uses replaced with Undef values.
- Any host function with target region code is marked to be removed during the the second stage.
- Calls to such functions are still removed and their uses replaced with Undef values.

Co-authored-by: Sergio Afonso sergio.afonsofumero@amd.com

llvmbot · 2023-11-09T19:13:58Z

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

@llvm/pr-subscribers-mlir-llvm

Author: Akash Banerjee (TIFitis)

Changes

Currently, when deleting the device functions in the second stage of filtering during MLIR to LLVM translation we can end up with invalid calls to these functions. This is because of the removal of the EarlyOutliningPass which would have otherwise gotten rid of any such calls.

This patch aims to alter the function filtering pass in the following way:
- Any host function is completely removed.
- Call to the host function are also removed and their uses replaced with Undef values.
- Any host function with target region code is marked to be removed during the the second stage.
- Calls to such functions are still removed and their uses replaced with Undef values.

Coauthored by: skatrak(Sergio.Afonso@amd.com)

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

6 Files Affected:

(modified) flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp (+25-12)
(added) flang/test/Driver/OpenMP/target-filtering.f90 (+37)
(modified) flang/test/Lower/OpenMP/FIR/array-bounds.f90 (+2-2)
(modified) flang/test/Lower/OpenMP/function-filtering.f90 (+1-2)
(modified) flang/test/Transforms/omp-function-filtering.mlir (+2-4)
(modified) mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp (-2)

diff --git a/flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp b/flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp
index 43fa5b7c4de2414..9e235c2d250fe93 100644
--- a/flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp
+++ b/flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp
@@ -33,6 +33,8 @@ class OMPFunctionFilteringPass
   OMPFunctionFilteringPass() = default;
 
   void runOnOperation() override {
+    MLIRContext *context = &getContext();
+    OpBuilder opBuilder(context);
     auto op = dyn_cast<omp::OffloadModuleInterface>(getOperation());
     if (!op || !op.getIsTargetDevice())
       return;
@@ -46,8 +48,6 @@ class OMPFunctionFilteringPass
               ->walk<WalkOrder::PreOrder>(
                   [&](omp::TargetOp) { return WalkResult::interrupt(); })
               .wasInterrupted();
-      if (hasTargetRegion)
-        return;
 
       omp::DeclareTargetDeviceType declareType =
           omp::DeclareTargetDeviceType::host;
@@ -56,17 +56,30 @@ class OMPFunctionFilteringPass
       if (declareTargetOp && declareTargetOp.isDeclareTarget())
         declareType = declareTargetOp.getDeclareTargetDeviceType();
 
-      // Filtering a function here means removing its body and explicitly
-      // setting its omp.declare_target attribute, so that following
-      // translation/lowering/transformation passes will skip processing its
-      // contents, but preventing the calls to undefined symbols that could
-      // result if the function were deleted. The second stage of function
-      // filtering, at the MLIR to LLVM IR translation level, will remove these
-      // from the IR thanks to the mismatch between the omp.declare_target
-      // attribute and the target device.
+      // Filtering a function here means deleting it if it doesn't containt a
+      // target region. Else we explicitly setting its omp.declare_target
+      // attribute. The second stage of function filtering at the MLIR to LLVM
+      // IR translation level will remove functions that contain the target
+      // region from the generated llvm IR.
       if (declareType == omp::DeclareTargetDeviceType::host) {
-        funcOp.eraseBody();
-        funcOp.setVisibility(SymbolTable::Visibility::Private);
+        auto funcUses = *funcOp.getSymbolUses(op);
+        for (auto use : funcUses) {
+          auto *callOp = use.getUser();
+          // If the callOp has users then replace them with Undef values.
+          if (!callOp->use_empty()) {
+            SmallVector<Value> undefResults;
+            for (auto res : callOp->getResults()) {
+              opBuilder.setInsertionPoint(callOp);
+              undefResults.emplace_back(
+                  opBuilder.create<fir::UndefOp>(res.getLoc(), res.getType()));
+            }
+            callOp->replaceAllUsesWith(undefResults);
+          }
+          // Remove the callOp
+          callOp->erase();
+        }
+        if (!hasTargetRegion)
+          funcOp.erase();
         if (declareTargetOp)
           declareTargetOp.setDeclareTarget(declareType,
                                            omp::DeclareTargetCaptureClause::to);
diff --git a/flang/test/Driver/OpenMP/target-filtering.f90 b/flang/test/Driver/OpenMP/target-filtering.f90
new file mode 100644
index 000000000000000..6e1bd34975c2728
--- /dev/null
+++ b/flang/test/Driver/OpenMP/target-filtering.f90
@@ -0,0 +1,37 @@
+!RUN: %flang_fc1 -emit-llvm -fopenmp %s -o - | FileCheck %s --check-prefixes HOST
+!RUN: %flang_fc1 -emit-llvm -fopenmp -fopenmp-is-target-device %s -o - | FileCheck %s --check-prefixes DEVICE
+
+!HOST: define {{.*}}@{{.*}}before{{.*}}(
+!DEVICE-NOT: define {{.*}}@before{{.*}}(
+!DEVICE-NOT: declare {{.*}}@before{{.*}}
+integer function before(x)
+integer, intent(in) :: x
+before = x + 200
+end function
+
+!HOST: define {{.*}}@{{.*}}main{{.*}}(
+!DEVICE: define {{.*}}@{{.*}}main{{.*}}(
+program main
+integer :: x, before, after
+!$omp target map(tofrom : x)
+   x = 100
+!$omp end target
+!HOST: call {{.*}}@{{.*}}before{{.*}}(
+!DEVICE-NOT: call {{.*}}@before{{.*}}(
+!HOST: call {{.*}}@{{.*}}after{{.*}}(
+!DEVICE-NOT: call {{.*}}@after{{.*}}(
+x = x + before(x) + after(x)
+WRITE(*,*) "Test:", x
+!$omp target map(tofrom : x)
+   x = 50
+!$omp end target
+WRITE(*,*) "Test:", x
+end program
+
+!HOST: define {{.*}}@{{.*}}after{{.*}}(
+!DEVICE-NOT: define {{.*}}@after{{.*}}(
+!DEVICE-NOT: declare {{.*}}@after{{.*}}
+integer function after(x)
+integer, intent(in) :: x
+after = x + 300
+end function
\ No newline at end of file
diff --git a/flang/test/Lower/OpenMP/FIR/array-bounds.f90 b/flang/test/Lower/OpenMP/FIR/array-bounds.f90
index abef31af22ba663..deb2e063f9db714 100644
--- a/flang/test/Lower/OpenMP/FIR/array-bounds.f90
+++ b/flang/test/Lower/OpenMP/FIR/array-bounds.f90
@@ -32,7 +32,7 @@ end subroutine read_write_section
 module assumed_array_routines
 contains
 !ALL-LABEL: func.func @_QMassumed_array_routinesPassumed_shape_array(
-!ALL-SAME: %[[ARG0:.*]]: !fir.box<!fir.array<?xi32>> {fir.bindc_name = "arr_read_write"}) {
+!ALL-SAME: %[[ARG0:.*]]: !fir.box<!fir.array<?xi32>> {fir.bindc_name = "arr_read_write"})
 !ALL: %[[ALLOCA:.*]] = fir.alloca i32 {bindc_name = "i", uniq_name = "_QMassumed_array_routinesFassumed_shape_arrayEi"}
 !ALL: %[[C0:.*]] = arith.constant 1 : index
 !ALL: %[[C1:.*]] = arith.constant 0 : index
@@ -56,7 +56,7 @@ subroutine assumed_shape_array(arr_read_write)
         end subroutine assumed_shape_array
 
 !ALL-LABEL:   func.func @_QMassumed_array_routinesPassumed_size_array(
-!ALL-SAME: %[[ARG0:.*]]: !fir.ref<!fir.array<?xi32>> {fir.bindc_name = "arr_read_write"}) {
+!ALL-SAME: %[[ARG0:.*]]: !fir.ref<!fir.array<?xi32>> {fir.bindc_name = "arr_read_write"})
 !ALL: %[[ALLOCA:.*]] = fir.alloca i32 {bindc_name = "i", uniq_name = "_QMassumed_array_routinesFassumed_size_arrayEi"}
 !ALL: %[[C0:.*]] = arith.constant 1 : index
 !ALL: %[[C1:.*]] = arith.constant 1 : index
diff --git a/flang/test/Lower/OpenMP/function-filtering.f90 b/flang/test/Lower/OpenMP/function-filtering.f90
index e550348e50692c5..11472bc1de957e3 100644
--- a/flang/test/Lower/OpenMP/function-filtering.f90
+++ b/flang/test/Lower/OpenMP/function-filtering.f90
@@ -21,8 +21,7 @@ end function device_fn
 
 ! MLIR-HOST: func.func @{{.*}}host_fn(
 ! MLIR-HOST: return
-! MLIR-DEVICE: func.func private @{{.*}}host_fn(
-! MLIR-DEVICE-NOT: return
+! MLIR-DEVICE-NOT: func.func {{.*}}host_fn(
 
 ! LLVM-HOST: define {{.*}} @{{.*}}host_fn{{.*}}(
 ! LLVM-DEVICE-NOT: {{.*}} @{{.*}}host_fn{{.*}}(
diff --git a/flang/test/Transforms/omp-function-filtering.mlir b/flang/test/Transforms/omp-function-filtering.mlir
index 44777e2cac30c50..b831968cb7dc6fd 100644
--- a/flang/test/Transforms/omp-function-filtering.mlir
+++ b/flang/test/Transforms/omp-function-filtering.mlir
@@ -4,10 +4,8 @@
 // CHECK: return
 // CHECK: func.func @nohost
 // CHECK: return
-// CHECK: func.func private @host
-// CHECK-NOT: return
-// CHECK: func.func private @none
-// CHECK-NOT: return
+// CHECK-NOT: func.func {{.*}}}} @host
+// CHECK-NOT: func.func {{.*}}}} @none
 // CHECK: func.func @nohost_target
 // CHECK: return
 // CHECK: func.func @host_target
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index d9ab785a082835d..4c1dc6603af1599 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -2496,8 +2496,6 @@ convertDeclareTargetAttr(Operation *op, mlir::omp::DeclareTargetAttr attribute,
       if (declareType == omp::DeclareTargetDeviceType::host) {
         llvm::Function *llvmFunc =
             moduleTranslation.lookupFunction(funcOp.getName());
-        llvmFunc->replaceAllUsesWith(
-            llvm::UndefValue::get(llvmFunc->getType()));
         llvmFunc->dropAllReferences();
         llvmFunc->eraseFromParent();
       }

llvmbot · 2023-11-09T19:13:58Z

@llvm/pr-subscribers-mlir-openmp

Author: Akash Banerjee (TIFitis)

Changes

Currently, when deleting the device functions in the second stage of filtering during MLIR to LLVM translation we can end up with invalid calls to these functions. This is because of the removal of the EarlyOutliningPass which would have otherwise gotten rid of any such calls.

This patch aims to alter the function filtering pass in the following way:
- Any host function is completely removed.
- Call to the host function are also removed and their uses replaced with Undef values.
- Any host function with target region code is marked to be removed during the the second stage.
- Calls to such functions are still removed and their uses replaced with Undef values.

Coauthored by: skatrak(Sergio.Afonso@amd.com)

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

6 Files Affected:

(modified) flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp (+25-12)
(added) flang/test/Driver/OpenMP/target-filtering.f90 (+37)
(modified) flang/test/Lower/OpenMP/FIR/array-bounds.f90 (+2-2)
(modified) flang/test/Lower/OpenMP/function-filtering.f90 (+1-2)
(modified) flang/test/Transforms/omp-function-filtering.mlir (+2-4)
(modified) mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp (-2)

diff --git a/flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp b/flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp
index 43fa5b7c4de2414..9e235c2d250fe93 100644
--- a/flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp
+++ b/flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp
@@ -33,6 +33,8 @@ class OMPFunctionFilteringPass
   OMPFunctionFilteringPass() = default;
 
   void runOnOperation() override {
+    MLIRContext *context = &getContext();
+    OpBuilder opBuilder(context);
     auto op = dyn_cast<omp::OffloadModuleInterface>(getOperation());
     if (!op || !op.getIsTargetDevice())
       return;
@@ -46,8 +48,6 @@ class OMPFunctionFilteringPass
               ->walk<WalkOrder::PreOrder>(
                   [&](omp::TargetOp) { return WalkResult::interrupt(); })
               .wasInterrupted();
-      if (hasTargetRegion)
-        return;
 
       omp::DeclareTargetDeviceType declareType =
           omp::DeclareTargetDeviceType::host;
@@ -56,17 +56,30 @@ class OMPFunctionFilteringPass
       if (declareTargetOp && declareTargetOp.isDeclareTarget())
         declareType = declareTargetOp.getDeclareTargetDeviceType();
 
-      // Filtering a function here means removing its body and explicitly
-      // setting its omp.declare_target attribute, so that following
-      // translation/lowering/transformation passes will skip processing its
-      // contents, but preventing the calls to undefined symbols that could
-      // result if the function were deleted. The second stage of function
-      // filtering, at the MLIR to LLVM IR translation level, will remove these
-      // from the IR thanks to the mismatch between the omp.declare_target
-      // attribute and the target device.
+      // Filtering a function here means deleting it if it doesn't containt a
+      // target region. Else we explicitly setting its omp.declare_target
+      // attribute. The second stage of function filtering at the MLIR to LLVM
+      // IR translation level will remove functions that contain the target
+      // region from the generated llvm IR.
       if (declareType == omp::DeclareTargetDeviceType::host) {
-        funcOp.eraseBody();
-        funcOp.setVisibility(SymbolTable::Visibility::Private);
+        auto funcUses = *funcOp.getSymbolUses(op);
+        for (auto use : funcUses) {
+          auto *callOp = use.getUser();
+          // If the callOp has users then replace them with Undef values.
+          if (!callOp->use_empty()) {
+            SmallVector<Value> undefResults;
+            for (auto res : callOp->getResults()) {
+              opBuilder.setInsertionPoint(callOp);
+              undefResults.emplace_back(
+                  opBuilder.create<fir::UndefOp>(res.getLoc(), res.getType()));
+            }
+            callOp->replaceAllUsesWith(undefResults);
+          }
+          // Remove the callOp
+          callOp->erase();
+        }
+        if (!hasTargetRegion)
+          funcOp.erase();
         if (declareTargetOp)
           declareTargetOp.setDeclareTarget(declareType,
                                            omp::DeclareTargetCaptureClause::to);
diff --git a/flang/test/Driver/OpenMP/target-filtering.f90 b/flang/test/Driver/OpenMP/target-filtering.f90
new file mode 100644
index 000000000000000..6e1bd34975c2728
--- /dev/null
+++ b/flang/test/Driver/OpenMP/target-filtering.f90
@@ -0,0 +1,37 @@
+!RUN: %flang_fc1 -emit-llvm -fopenmp %s -o - | FileCheck %s --check-prefixes HOST
+!RUN: %flang_fc1 -emit-llvm -fopenmp -fopenmp-is-target-device %s -o - | FileCheck %s --check-prefixes DEVICE
+
+!HOST: define {{.*}}@{{.*}}before{{.*}}(
+!DEVICE-NOT: define {{.*}}@before{{.*}}(
+!DEVICE-NOT: declare {{.*}}@before{{.*}}
+integer function before(x)
+integer, intent(in) :: x
+before = x + 200
+end function
+
+!HOST: define {{.*}}@{{.*}}main{{.*}}(
+!DEVICE: define {{.*}}@{{.*}}main{{.*}}(
+program main
+integer :: x, before, after
+!$omp target map(tofrom : x)
+   x = 100
+!$omp end target
+!HOST: call {{.*}}@{{.*}}before{{.*}}(
+!DEVICE-NOT: call {{.*}}@before{{.*}}(
+!HOST: call {{.*}}@{{.*}}after{{.*}}(
+!DEVICE-NOT: call {{.*}}@after{{.*}}(
+x = x + before(x) + after(x)
+WRITE(*,*) "Test:", x
+!$omp target map(tofrom : x)
+   x = 50
+!$omp end target
+WRITE(*,*) "Test:", x
+end program
+
+!HOST: define {{.*}}@{{.*}}after{{.*}}(
+!DEVICE-NOT: define {{.*}}@after{{.*}}(
+!DEVICE-NOT: declare {{.*}}@after{{.*}}
+integer function after(x)
+integer, intent(in) :: x
+after = x + 300
+end function
\ No newline at end of file
diff --git a/flang/test/Lower/OpenMP/FIR/array-bounds.f90 b/flang/test/Lower/OpenMP/FIR/array-bounds.f90
index abef31af22ba663..deb2e063f9db714 100644
--- a/flang/test/Lower/OpenMP/FIR/array-bounds.f90
+++ b/flang/test/Lower/OpenMP/FIR/array-bounds.f90
@@ -32,7 +32,7 @@ end subroutine read_write_section
 module assumed_array_routines
 contains
 !ALL-LABEL: func.func @_QMassumed_array_routinesPassumed_shape_array(
-!ALL-SAME: %[[ARG0:.*]]: !fir.box<!fir.array<?xi32>> {fir.bindc_name = "arr_read_write"}) {
+!ALL-SAME: %[[ARG0:.*]]: !fir.box<!fir.array<?xi32>> {fir.bindc_name = "arr_read_write"})
 !ALL: %[[ALLOCA:.*]] = fir.alloca i32 {bindc_name = "i", uniq_name = "_QMassumed_array_routinesFassumed_shape_arrayEi"}
 !ALL: %[[C0:.*]] = arith.constant 1 : index
 !ALL: %[[C1:.*]] = arith.constant 0 : index
@@ -56,7 +56,7 @@ subroutine assumed_shape_array(arr_read_write)
         end subroutine assumed_shape_array
 
 !ALL-LABEL:   func.func @_QMassumed_array_routinesPassumed_size_array(
-!ALL-SAME: %[[ARG0:.*]]: !fir.ref<!fir.array<?xi32>> {fir.bindc_name = "arr_read_write"}) {
+!ALL-SAME: %[[ARG0:.*]]: !fir.ref<!fir.array<?xi32>> {fir.bindc_name = "arr_read_write"})
 !ALL: %[[ALLOCA:.*]] = fir.alloca i32 {bindc_name = "i", uniq_name = "_QMassumed_array_routinesFassumed_size_arrayEi"}
 !ALL: %[[C0:.*]] = arith.constant 1 : index
 !ALL: %[[C1:.*]] = arith.constant 1 : index
diff --git a/flang/test/Lower/OpenMP/function-filtering.f90 b/flang/test/Lower/OpenMP/function-filtering.f90
index e550348e50692c5..11472bc1de957e3 100644
--- a/flang/test/Lower/OpenMP/function-filtering.f90
+++ b/flang/test/Lower/OpenMP/function-filtering.f90
@@ -21,8 +21,7 @@ end function device_fn
 
 ! MLIR-HOST: func.func @{{.*}}host_fn(
 ! MLIR-HOST: return
-! MLIR-DEVICE: func.func private @{{.*}}host_fn(
-! MLIR-DEVICE-NOT: return
+! MLIR-DEVICE-NOT: func.func {{.*}}host_fn(
 
 ! LLVM-HOST: define {{.*}} @{{.*}}host_fn{{.*}}(
 ! LLVM-DEVICE-NOT: {{.*}} @{{.*}}host_fn{{.*}}(
diff --git a/flang/test/Transforms/omp-function-filtering.mlir b/flang/test/Transforms/omp-function-filtering.mlir
index 44777e2cac30c50..b831968cb7dc6fd 100644
--- a/flang/test/Transforms/omp-function-filtering.mlir
+++ b/flang/test/Transforms/omp-function-filtering.mlir
@@ -4,10 +4,8 @@
 // CHECK: return
 // CHECK: func.func @nohost
 // CHECK: return
-// CHECK: func.func private @host
-// CHECK-NOT: return
-// CHECK: func.func private @none
-// CHECK-NOT: return
+// CHECK-NOT: func.func {{.*}}}} @host
+// CHECK-NOT: func.func {{.*}}}} @none
 // CHECK: func.func @nohost_target
 // CHECK: return
 // CHECK: func.func @host_target
diff --git a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
index d9ab785a082835d..4c1dc6603af1599 100644
--- a/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
@@ -2496,8 +2496,6 @@ convertDeclareTargetAttr(Operation *op, mlir::omp::DeclareTargetAttr attribute,
       if (declareType == omp::DeclareTargetDeviceType::host) {
         llvm::Function *llvmFunc =
             moduleTranslation.lookupFunction(funcOp.getName());
-        llvmFunc->replaceAllUsesWith(
-            llvm::UndefValue::get(llvmFunc->getType()));
         llvmFunc->dropAllReferences();
         llvmFunc->eraseFromParent();
       }

flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp

agozillon

LGTM, I've tested this locally and it does indeed fix the cases specified. However, please do wait for another reviewer to approve as well (just to have a second set of eyes)!

skatrak

Thank you Akash for this work. My suggestions are mostly for small nits and improvements to the unit tests, except for one case that may currently not be handled correctly. If I'm wrong and it already works, I think it would still be a good idea to add a test case for it.

flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp

flang/test/Driver/OpenMP/target-filtering.f90

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

flang/test/Lower/OpenMP/function-filtering.f90

flang/test/Transforms/omp-function-filtering.mlir

TIFitis · 2023-11-10T16:44:23Z

I've updated the patch to address all the reviewer comments.

flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp

banach-space · 2023-11-11T12:04:46Z

flang/test/Driver/OpenMP/target-filtering.f90

@@ -0,0 +1,61 @@
+!RUN: %flang_fc1 -emit-llvm -fopenmp %s -o - | FileCheck %s --check-prefixes HOST,ALL


Is this really a driver test? This patch doesn't add any driver functionality, so there should be no driver tests, right?

Looks like all you want is to verify the lowering to LLVM, but that's different to testing the driver. Please could you move this file? I mostly want to avoid @pr-subscribers-flang-driver being tagged when no driver changes are made. Thanks!

@kiranchandramohan previously suggested flang/test/Driver/OpenMP/ as a potential directory to keep lowering to llvm IR tests. Can you please suggest a better alternative for these tests?

The right way to do LIT tests is to test a single stage at a time. You can use the following flow as guidance.

For tests lowering to HLFIR + FIR + OpenMP from source use flang/test/Lower/OpenMP.

For tests converting from FIR + OpenMP to LLVM dialect use flang/test/Fir/convert-to-llvm-openmp-and-fir.fir

For tests converting form LLVM + OpenMP or core-dialect+OpenMP to LLVM + OpenMP use mlir/test/Conversion/OpenMPToLLVM/

For test converting from LLVM + OpenMP to LLVM IR use mlir/test/Target/LLVMIR/openmp-*

If you are testing a specific pass then the tests should go to flang/test/Transforms/. If there are many OpenMP related tests you can create a sub-directory there. But note that the test here should be from FIR+OpenMP to FIR+OpenMP.

We do already have tests for the partial stages.

I felt that we should have some end-to-end tests for things that rely on multiple stages. If you feel like these are unnecessary then I can get rid of the test being added in this patch, and put up a separate patch to remove the remainder.

You can write an RFC to have flang/test/llvm-ir or flang/test/end-to-end get approval and then add tests there.
You can also add end to end and execution tests in https://github.com/llvm/llvm-test-suite/tree/main/Fortran.

If you feel like these are unnecessary

I did not say this.

I've put up an RFC here https://discourse.llvm.org/t/rfc-flang-new-directory-for-adding-end-to-end-tests-for-lowering-to-llvm-ir-in-flang/74872.

Shall I let the flang/test/Driver/OpenMP/target-filtering.f90 test be there for now in the Driver? Or I can remove it and add later based on the RFC.

I've removed this test for now so that the patch may land. Once the RFC is settled, I'll add the test accordingly.

skatrak

LGTM! I'd wait for a successful pre-merge buildbot run before merging, but in my opinion it should be ready to go. Thanks for the work @TIFitis.

flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp

Currently, when deleting the device functions in the second stage of filtering during MLIR to LLVM translation we can end up with invalid calls to these functions. This is because of the removal of the EarlyOutliningPass which would have otherwise gotten rid of any such calls. This patch aims to alter the function filtering pass in the following way: - Any host function is completely removed. - Call to the host function are also removed and their uses replaced with Undef values. - Any host function with target region code is marked to be removed during the the second stage. - Calls to such functions are still removed and their uses replaced with Undef values. Co-authored-by: Sergio Afonso <sergio.afonsofumero@amd.com>

TIFitis requested review from skatrak, jsjodin, kiranchandramohan and agozillon November 9, 2023 19:13

llvmbot added mlir:llvm flang:driver mlir flang Flang issues not falling into any other category mlir:openmp flang:fir-hlfir flang:openmp labels Nov 9, 2023

agozillon reviewed Nov 10, 2023

View reviewed changes

flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp Outdated Show resolved Hide resolved

agozillon approved these changes Nov 10, 2023

View reviewed changes

skatrak requested changes Nov 10, 2023

View reviewed changes

TIFitis force-pushed the omp-filtering branch from 6edef3a to b36fdd2 Compare November 10, 2023 16:43

TIFitis commented Nov 10, 2023

View reviewed changes

flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp Outdated Show resolved Hide resolved

banach-space reviewed Nov 11, 2023

View reviewed changes

TIFitis force-pushed the omp-filtering branch 2 times, most recently from 7b2149f to 71458a4 Compare November 14, 2023 11:52

skatrak approved these changes Nov 14, 2023

View reviewed changes

flang/lib/Optimizer/Transforms/OMPFunctionFiltering.cpp Outdated Show resolved Hide resolved

TIFitis force-pushed the omp-filtering branch from 71458a4 to 8b102e5 Compare November 14, 2023 12:06

TIFitis merged commit 8701b17 into llvm:main Nov 14, 2023
3 checks passed

TIFitis deleted the omp-filtering branch November 15, 2023 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MLIR][OpenMP] Changes to function-filtering pass #71850

[MLIR][OpenMP] Changes to function-filtering pass #71850

TIFitis commented Nov 9, 2023 •

edited

Loading

llvmbot commented Nov 9, 2023 •

edited

Loading

llvmbot commented Nov 9, 2023

agozillon left a comment

skatrak left a comment

TIFitis commented Nov 10, 2023

banach-space Nov 11, 2023

TIFitis Nov 13, 2023

kiranchandramohan Nov 13, 2023

kiranchandramohan Nov 13, 2023

TIFitis Nov 13, 2023

kiranchandramohan Nov 13, 2023

TIFitis Nov 13, 2023

TIFitis Nov 14, 2023

skatrak left a comment

		@@ -0,0 +1,61 @@
		!RUN: %flang_fc1 -emit-llvm -fopenmp %s -o - \| FileCheck %s --check-prefixes HOST,ALL

[MLIR][OpenMP] Changes to function-filtering pass #71850

[MLIR][OpenMP] Changes to function-filtering pass #71850

Conversation

TIFitis commented Nov 9, 2023 • edited Loading

llvmbot commented Nov 9, 2023 • edited Loading

llvmbot commented Nov 9, 2023

agozillon left a comment

Choose a reason for hiding this comment

skatrak left a comment

Choose a reason for hiding this comment

TIFitis commented Nov 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skatrak left a comment

Choose a reason for hiding this comment

TIFitis commented Nov 9, 2023 •

edited

Loading

llvmbot commented Nov 9, 2023 •

edited

Loading