[flang] Apply nocapture attribute to dummy arguments #116182

s-watanabe314 · 2024-11-14T08:35:22Z

Apply llvm.nocapture attribute to dummy arguments that do not have the target, asynchronous, volatile, or pointer attributes in a procedure that is not a bind(c). This was discussed in

https://discourse.llvm.org/t/applying-the-nocapture-attribute-to-reference-passed-arguments-in-fortran-subroutines/81401

Apply llvm.nocapture attribute to dummy arguments that do not have the target, asynchronous, volatile, or pointer attributes in a procedure that is not a bind(c). This was discussed in https://discourse.llvm.org/t/applying-the-nocapture-attribute-to-reference-passed-arguments-in-fortran-subroutines/81401

llvmbot · 2024-11-14T08:36:02Z

@llvm/pr-subscribers-flang-openmp
@llvm/pr-subscribers-flang-driver

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

Author: None (s-watanabe314)

Changes

Apply llvm.nocapture attribute to dummy arguments that do not have the target, asynchronous, volatile, or pointer attributes in a procedure that is not a bind(c). This was discussed in

https://discourse.llvm.org/t/applying-the-nocapture-attribute-to-reference-passed-arguments-in-fortran-subroutines/81401

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

2 Files Affected:

(modified) flang/lib/Optimizer/Transforms/FunctionAttr.cpp (+21)
(added) flang/test/Transforms/function-attrs.fir (+45)

diff --git a/flang/lib/Optimizer/Transforms/FunctionAttr.cpp b/flang/lib/Optimizer/Transforms/FunctionAttr.cpp
index 69c41595974ef9..8b1aef38cc0267 100644
--- a/flang/lib/Optimizer/Transforms/FunctionAttr.cpp
+++ b/flang/lib/Optimizer/Transforms/FunctionAttr.cpp
@@ -10,6 +10,8 @@
 /// \file
 /// This is a generic pass for adding attributes to functions.
 //===----------------------------------------------------------------------===//
+#include "flang/Optimizer/Dialect/FIROpsSupport.h"
+#include "flang/Optimizer/Support/InternalNames.h"
 #include "flang/Optimizer/Transforms/Passes.h"
 #include "mlir/Dialect/LLVMIR/LLVMAttrs.h"
 #include "mlir/Dialect/LLVMIR/LLVMDialect.h"
@@ -45,6 +47,25 @@ void FunctionAttrPass::runOnOperation() {
 
   LLVM_DEBUG(llvm::dbgs() << "Func-name:" << func.getSymName() << "\n");
 
+  llvm::StringRef name = func.getSymName();
+  auto deconstructed = fir::NameUniquer::deconstruct(name);
+  bool isFromModule = !deconstructed.second.modules.empty();
+
+  if ((isFromModule || !func.isDeclaration()) &&
+      !fir::hasBindcAttr(func.getOperation())) {
+    llvm::StringRef nocapture = mlir::LLVM::LLVMDialect::getNoCaptureAttrName();
+    mlir::UnitAttr unitAttr = mlir::UnitAttr::get(func.getContext());
+
+    for (auto [index, argType] : llvm::enumerate(func.getArgumentTypes())) {
+      if (mlir::isa<fir::ReferenceType>(argType) &&
+          !fir::isPointerType(argType) &&
+          !func.getArgAttr(index, fir::getTargetAttrName()) &&
+          !func.getArgAttr(index, fir::getAsynchronousAttrName()) &&
+          !func.getArgAttr(index, fir::getVolatileAttrName()))
+        func.setArgAttr(index, nocapture, unitAttr);
+    }
+  }
+
   mlir::MLIRContext *context = &getContext();
   if (framePointerKind != mlir::LLVM::framePointerKind::FramePointerKind::None)
     func->setAttr("frame_pointer", mlir::LLVM::FramePointerKindAttr::get(
diff --git a/flang/test/Transforms/function-attrs.fir b/flang/test/Transforms/function-attrs.fir
new file mode 100644
index 00000000000000..54ee8d007692c0
--- /dev/null
+++ b/flang/test/Transforms/function-attrs.fir
@@ -0,0 +1,45 @@
+// RUN: fir-opt --function-attr %s | FileCheck %s
+
+// If a function has a body and is not bind(c), and if the dummy argument doesn't have the target,
+// asynchronous, volatile, or pointer attribute, then add llvm.nocapture to the dummy argument.
+
+func.func @_QParg_nocapture(%arg0: !fir.ref<i32> {fir.bindc_name = "tar", fir.target}, %arg1: !fir.ref<i32> {fir.asynchronous, fir.bindc_name = "asynch"}, %arg2: !fir.ref<i32> {fir.bindc_name = "vol", fir.volatile}, %arg3: !fir.ref<!fir.box<!fir.ptr<i32>>> {fir.bindc_name = "ptr"}, %arg4: !fir.ref<i32> {fir.bindc_name = "nocap"}) {
+    %0 = fir.dummy_scope : !fir.dscope
+    %1 = fir.declare %arg0 dummy_scope %0 {fortran_attrs = #fir.var_attrs<target>, uniq_name = "_QFarg_nocaptureEtar"} : (!fir.ref<i32>, !fir.dscope) -> !fir.ref<i32>
+    %2 = fir.declare %arg1 dummy_scope %0 {fortran_attrs = #fir.var_attrs<asynchronous>, uniq_name = "_QFarg_nocaptureEasynch"} : (!fir.ref<i32>, !fir.dscope) -> !fir.ref<i32>
+    %3 = fir.declare %arg2 dummy_scope %0 {uniq_name = "_QFarg_nocaptureEvol"} : (!fir.ref<i32>, !fir.dscope) -> !fir.ref<i32>
+    %4 = fir.declare %arg3 dummy_scope %0 {fortran_attrs = #fir.var_attrs<pointer>, uniq_name = "_QFarg_nocaptureEptr"} : (!fir.ref<!fir.box<!fir.ptr<i32>>>, !fir.dscope) -> !fir.ref<!fir.box<!fir.ptr<i32>>>
+    %5 = fir.declare %arg4 dummy_scope %0 {uniq_name = "_QFarg_nocaptureEnocap"} : (!fir.ref<i32>, !fir.dscope) -> !fir.ref<i32>
+    return
+}
+// CHECK-LABEL: func.func @_QParg_nocapture(
+// CHECK-SAME:                                 %[[ARG0:.*]]: !fir.ref<i32> {fir.bindc_name = "tar", fir.target},
+// CHECK-SAME:                                 %[[ARG1:.*]]: !fir.ref<i32> {fir.asynchronous, fir.bindc_name = "asynch"},
+// CHECK-SAME:                                 %[[ARG2:.*]]: !fir.ref<i32> {fir.bindc_name = "vol", fir.volatile},
+// CHECK-SAME:                                 %[[ARG3:.*]]: !fir.ref<!fir.box<!fir.ptr<i32>>> {fir.bindc_name = "ptr"},
+// CHECK-SAME:                                 %[[ARG4:.*]]: !fir.ref<i32> {fir.bindc_name = "nocap", llvm.nocapture}) {
+// CHECK:    return
+// CHECK-NEXT: }
+
+func.func @arg_nocapture_bindc(%arg0: !fir.ref<i32> {fir.bindc_name = "tar", fir.target}, %arg1: !fir.ref<i32> {fir.bindc_name = "nocap"}) attributes {fir.bindc_name = "arg_nocapture_bindc", fir.proc_attrs = #fir.proc_attrs<bind_c>} {
+    %0 = fir.dummy_scope : !fir.dscope
+    %1 = fir.declare %arg0 dummy_scope %0 {fortran_attrs = #fir.var_attrs<target>, uniq_name = "_QFarg_nocapture_bindcEtar"} : (!fir.ref<i32>, !fir.dscope) -> !fir.ref<i32>
+    %2 = fir.declare %arg1 dummy_scope %0 {uniq_name = "_QFarg_nocapture_bindcEnocap"} : (!fir.ref<i32>, !fir.dscope) -> !fir.ref<i32>
+    return
+}
+// CHECK-LABEL: func.func @arg_nocapture_bindc(
+// CHECK-NOT:  llvm.nocapture
+
+
+// If a function declaration is from a module and is not bind(c), and if the dummy argument doesn't have
+// the target, asynchronous, volatile, or pointer attribute, then add llvm.nocapture to the dummy argument.
+
+func.func private @_QMarg_modPcheck_args(!fir.ref<i32> {fir.target}, !fir.ref<i32> {fir.asynchronous}, !fir.ref<i32> {fir.volatile}, !fir.ref<!fir.box<!fir.ptr<i32>>>, !fir.ref<i32>, !fir.boxchar<1>, !fir.ref<complex<f32>>)
+// CHECK-LABEL: func.func private @_QMarg_modPcheck_args(
+// CHECK-SAME:                                 !fir.ref<i32> {fir.target},
+// CHECK-SAME:                                 !fir.ref<i32> {fir.asynchronous},
+// CHECK-SAME:                                 !fir.ref<i32> {fir.volatile},
+// CHECK-SAME:                                 !fir.ref<!fir.box<!fir.ptr<i32>>>,
+// CHECK-SAME:                                 !fir.ref<i32> {llvm.nocapture},
+// CHECK-SAME:                                 !fir.boxchar<1>,
+// CHECK-SAME:                                 !fir.ref<complex<f32>> {llvm.nocapture})

tblah

Looks good to me. Thanks!

jeanPerier

The change in the pass looks good to me, but you should probably enable this pass by default (or at least when O > O0, opinions welcomed here) because it is currently only run under a specific set of conditions (see

llvm-project/flang/lib/Optimizer/Passes/Pipelines.cpp

Line 278 in 40afff7

if (config.FramePointerKind != llvm::FramePointerKind::None ||

).

Currently, if one run the patch with flang -fc1 -emit-llvm -O2 on the code below the pass is not run and the attribute not added.

subroutine foo(x)
  call bar(x)
end subroutine

jeanPerier · 2024-11-15T13:05:06Z

flang/lib/Optimizer/Transforms/FunctionAttr.cpp

+
+    for (auto [index, argType] : llvm::enumerate(func.getArgumentTypes())) {
+      if (mlir::isa<fir::ReferenceType>(argType) &&
+          !fir::isPointerType(argType) &&


You do not need the fir::isPointerType check here: it prevents putting nocapture on fir.ref<fir.box<fir.ptr<>> (addresses of descriptor created for Fortran POINTERs). It would be incorrect to add nocapture on the base address of the POINTER (which is the first struct member of the descriptor), but the address of the descriptor itself cannot be taken in Fortran.

Thank you for the review.
I thought that since the dummy argument defined as integer, pointer :: x is translated to fir.ref<fir.box<fir.ptr<i32>>> (https://godbolt.org/z/qbYn59d6M), it could be determined using fir::isPointerType.
As another way to determine whether a dummy argument has the pointer attribute, should I add the fir.pointer attribute, similar to target and asynchronous, and check it with !func.getArgAttr(index, fir::getPointerAttrName())?

What I am pointing out is not the ability to detect or not the the argument is for a Fortran pointer, it is that Fotran Pointers are always passed as the address of a descriptor, and while the address inside the descriptor may be captured because it is the target of a Fortran POINTER, the address of the descriptor cannot be taken (there is no way to do that in Fortran). The llvm nocapture applies to the address that is the function argument, which in this case is the one of the descriptor, not the base address, so it is still correct to add nocapture to a fir.ref<fir.box<fir.ptr<>>.

Let me use C to illustrate here: when passing a Fortran pointer, the ABI looks like:

typedef struct { void* base_address; // ... more fields } Descriptor; void foo(Descriptor* fortran_pointer);

Fortran tells you that fortran_pointer->base_address may be taken inside foo, but fortran_pointer address cannot. As far as I understand llvm.nocapture, it applies to the SSA argument address, not the addresses inside the memory pointed to by the SSA argument address. Descriptor* is the C equivalent of fir.ref<fir.box<>>.

Thank you for the detailed explanation. I understand that applying nocapture is correct because it's a descriptor. I've removed fir::isPointerType and corrected the test.

s-watanabe314 · 2024-11-19T07:54:34Z

The change in the pass looks good to me, but you should probably enable this pass by default (or at least when O > O0, opinions welcomed here) because it is currently only run under a specific set of conditions (see

llvm-project/flang/lib/Optimizer/Passes/Pipelines.cpp

Line 278 in 40afff7

if (config.FramePointerKind != llvm::FramePointerKind::None ||

).
Currently, if one run the patch with flang -fc1 -emit-llvm -O2 on the code below the pass is not run and the attribute not added.
subroutine foo(x)
  call bar(x)
end subroutine

Thank you for your review. I'll modify the code so that the path is enabled by default.
After making the changes below, a dozen or so regression tests failed, so I'll confirm them before committing.

  // Add function attributes
  mlir::LLVM::framePointerKind::FramePointerKind framePointerKind;

  if (config.FramePointerKind == llvm::FramePointerKind::NonLeaf)
    framePointerKind = mlir::LLVM::framePointerKind::FramePointerKind::NonLeaf;
  else if (config.FramePointerKind == llvm::FramePointerKind::All)
    framePointerKind = mlir::LLVM::framePointerKind::FramePointerKind::All;
  else
    framePointerKind = mlir::LLVM::framePointerKind::FramePointerKind::None;

  pm.addPass(fir::createFunctionAttr(
      {framePointerKind, config.NoInfsFPMath, config.NoNaNsFPMath,
       config.ApproxFuncFPMath, config.NoSignedZerosFPMath,
       config.UnsafeFPMath}));

s-watanabe314 · 2024-11-20T07:20:18Z

I'm not sure how to handle the regression in the Fir/box.fir test when FunctionAttrPass is modified to run by default. Could you advise me on this?

llvm-project/flang/test/Fir/box.fir

Lines 55 to 58 in 40afff7

    
           // Boxing of a scalar character of dynamic length 
        
           // CHECK-LABEL: define void @b1( 
        
           // CHECK-SAME: ptr %[[res:.*]], ptr %[[arg0:.*]], i64 %[[arg1:.*]]) 
        
           func.func @b1(%arg0 : !fir.ref<!fir.char<1,?>>, %arg1 : index) -> !fir.box<!fir.char<1,?>> {

It seems that the return value of function @b1 is converted to a dummy argument(ptr %[[res:.*]]) by the CodeGenRewritePass, and nocapture is applied to it. Is it correct to apply nocapture to a dummy argument that was originally a return value?

jeanPerier · 2024-11-20T09:44:48Z

Is it correct to apply nocapture to a dummy argument that was originally a return value?

It actually seems correct to me to add nocapture to hidden arguments of results because pointer associations made with the result entity inside the procedure should be undefined after the call (The standard reference for that should be somewhere in F2023 19.5.2.5, I cannot find what exactly, but it would make no sense otherwise. @klausler do you agree?).

klausler · 2024-11-20T16:32:02Z

Is it correct to apply nocapture to a dummy argument that was originally a return value?

It actually seems correct to me to add nocapture to hidden arguments of results because pointer associations made with the result entity inside the procedure should be undefined after the call (The standard reference for that should be somewhere in F2023 19.5.2.5, I cannot find what exactly, but it would make no sense otherwise. @klausler do you agree?).

Function result variables don't seem to me to be any different from other local variables in this respect.

s-watanabe314 · 2024-11-21T09:48:48Z

Thank you for your comments.
I understand that adding nocapture is correct and have fixed the test.
I've committed the changes, so please review them.

jeanPerier

LGTM, thanks

s-watanabe314 · 2024-11-28T06:37:25Z

Thank you for the approval! I'll merge it.

I'm not sure how to handle the regression in the Fir/box.fir test when FunctionAttrPass is modified to run by default. Could you advise me on this?

llvm-project/flang/test/Fir/box.fir

Lines 55 to 58 in 40afff7

// Boxing of a scalar character of dynamic length

// CHECK-LABEL: define void @b1(

// CHECK-SAME: ptr %[[res:.*]], ptr %[[arg0:.*]], i64 %[[arg1:.*]])

func.func @b1(%arg0 : !fir.ref<!fir.char<1,?>>, %arg1 : index) -> !fir.box<!fir.char<1,?>> {

It seems that the return value of function @b1 is converted to a dummy argument(ptr %[[res:.*]]) by the CodeGenRewritePass, and nocapture is applied to it. Is it correct to apply nocapture to a dummy argument that was originally a return value?

I previously asked about the transformation that moves the return value to an argument, but I don't understand the purpose of this transformation. Could someone please explain it to me?

llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Nov 14, 2024

s-watanabe314 requested review from tblah, jeanPerier and vzakhari November 14, 2024 09:09

tblah approved these changes Nov 14, 2024

View reviewed changes

jeanPerier reviewed Nov 15, 2024

View reviewed changes

Remove isPointerType to apply nocapture to descriptor

a14805a

llvmbot added flang:driver flang:openmp labels Nov 21, 2024

Fix the pass pipeline to run FunctionAttrPass by default

f3ed71d

s-watanabe314 force-pushed the apply-nocapture-attr branch from ff7fceb to f3ed71d Compare November 21, 2024 09:21

jeanPerier approved these changes Nov 27, 2024

View reviewed changes

s-watanabe314 merged commit f3cf24f into llvm:main Nov 28, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[flang] Apply nocapture attribute to dummy arguments #116182

[flang] Apply nocapture attribute to dummy arguments #116182

Uh oh!

s-watanabe314 commented Nov 14, 2024

Uh oh!

llvmbot commented Nov 14, 2024 •

edited

Loading

Uh oh!

tblah left a comment

Uh oh!

jeanPerier left a comment

Uh oh!

jeanPerier Nov 15, 2024

Uh oh!

s-watanabe314 Nov 18, 2024

Uh oh!

jeanPerier Nov 18, 2024

Uh oh!

s-watanabe314 Nov 19, 2024 •

edited

Loading

Uh oh!

s-watanabe314 commented Nov 19, 2024

Uh oh!

s-watanabe314 commented Nov 20, 2024

Uh oh!

jeanPerier commented Nov 20, 2024

Uh oh!

klausler commented Nov 20, 2024

Uh oh!

s-watanabe314 commented Nov 21, 2024

Uh oh!

jeanPerier left a comment

Uh oh!

s-watanabe314 commented Nov 28, 2024

Uh oh!

Uh oh!

Uh oh!

[flang] Apply nocapture attribute to dummy arguments #116182

[flang] Apply nocapture attribute to dummy arguments #116182

Uh oh!

Conversation

s-watanabe314 commented Nov 14, 2024

Uh oh!

llvmbot commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tblah left a comment

Choose a reason for hiding this comment

Uh oh!

jeanPerier left a comment

Choose a reason for hiding this comment

Uh oh!

jeanPerier Nov 15, 2024

Choose a reason for hiding this comment

Uh oh!

s-watanabe314 Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

jeanPerier Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

s-watanabe314 Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-watanabe314 commented Nov 19, 2024

Uh oh!

s-watanabe314 commented Nov 20, 2024

Uh oh!

jeanPerier commented Nov 20, 2024

Uh oh!

klausler commented Nov 20, 2024

Uh oh!

s-watanabe314 commented Nov 21, 2024

Uh oh!

jeanPerier left a comment

Choose a reason for hiding this comment

Uh oh!

s-watanabe314 commented Nov 28, 2024

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Nov 14, 2024 •

edited

Loading

s-watanabe314 Nov 19, 2024 •

edited

Loading