Skip to content

[flang][extension] support concatenation with absent optional #112678

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

jeanPerier
Copy link
Contributor

Fix #112593 by adding support in lowering to concatenation with an absent optional assumed length dummy argument because:

  1. Most compilers seem to support it (most likely by accident).
  2. This actually makes the compiler codegen simpler. Codegen was going out of its way to poke the LLVM optimizer bear by producing an undef argument for the length.

I insist on the fact that no compiler support this with explicit length optional arguments and the executable will segafult and I would discourage users from using that "feature" because runtime checks for bad optional dereference will kick when used (For instance, "nagfor -C=present" will produce an executable that abort with an error message . Flang does not have such runtime check option so far).

@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:codegen labels Oct 17, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 17, 2024

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

Author: None (jeanPerier)

Changes

Fix #112593 by adding support in lowering to concatenation with an absent optional assumed length dummy argument because:

  1. Most compilers seem to support it (most likely by accident).
  2. This actually makes the compiler codegen simpler. Codegen was going out of its way to poke the LLVM optimizer bear by producing an undef argument for the length.

I insist on the fact that no compiler support this with explicit length optional arguments and the executable will segafult and I would discourage users from using that "feature" because runtime checks for bad optional dereference will kick when used (For instance, "nagfor -C=present" will produce an executable that abort with an error message . Flang does not have such runtime check option so far).


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

2 Files Affected:

  • (modified) flang/lib/Optimizer/CodeGen/CodeGen.cpp (+1-13)
  • (modified) flang/test/Fir/optional.fir (+1-1)
diff --git a/flang/lib/Optimizer/CodeGen/CodeGen.cpp b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
index 9b624efa053813..68b8c6613585e6 100644
--- a/flang/lib/Optimizer/CodeGen/CodeGen.cpp
+++ b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
@@ -3373,19 +3373,7 @@ struct AbsentOpConversion : public fir::FIROpConversion<fir::AbsentOp> {
   matchAndRewrite(fir::AbsentOp absent, OpAdaptor,
                   mlir::ConversionPatternRewriter &rewriter) const override {
     mlir::Type ty = convertType(absent.getType());
-    mlir::Location loc = absent.getLoc();
-
-    if (mlir::isa<fir::BoxCharType>(absent.getType())) {
-      auto structTy = mlir::cast<mlir::LLVM::LLVMStructType>(ty);
-      assert(!structTy.isOpaque() && !structTy.getBody().empty());
-      auto undefStruct = rewriter.create<mlir::LLVM::UndefOp>(loc, ty);
-      auto nullField =
-          rewriter.create<mlir::LLVM::ZeroOp>(loc, structTy.getBody()[0]);
-      rewriter.replaceOpWithNewOp<mlir::LLVM::InsertValueOp>(
-          absent, undefStruct, nullField, 0);
-    } else {
-      rewriter.replaceOpWithNewOp<mlir::LLVM::ZeroOp>(absent, ty);
-    }
+    rewriter.replaceOpWithNewOp<mlir::LLVM::ZeroOp>(absent, ty);
     return mlir::success();
   }
 };
diff --git a/flang/test/Fir/optional.fir b/flang/test/Fir/optional.fir
index 3b350d6fa94195..bded8b5332a300 100644
--- a/flang/test/Fir/optional.fir
+++ b/flang/test/Fir/optional.fir
@@ -47,7 +47,7 @@ func.func @foo3(%arg0: !fir.boxchar<1>) -> i1 {
 // CHECK-LABEL: @bar3
 func.func @bar3() -> i1 {
   %0 = fir.absent !fir.boxchar<1>
-  // CHECK: call i1 @foo3(ptr null, i64 undef)
+  // CHECK: call i1 @foo3(ptr null, i64 0)
   %1 = fir.call @foo3(%0) : (!fir.boxchar<1>) -> i1
   return %1 : i1
 }

@llvmbot
Copy link
Member

llvmbot commented Oct 17, 2024

@llvm/pr-subscribers-flang-codegen

Author: None (jeanPerier)

Changes

Fix #112593 by adding support in lowering to concatenation with an absent optional assumed length dummy argument because:

  1. Most compilers seem to support it (most likely by accident).
  2. This actually makes the compiler codegen simpler. Codegen was going out of its way to poke the LLVM optimizer bear by producing an undef argument for the length.

I insist on the fact that no compiler support this with explicit length optional arguments and the executable will segafult and I would discourage users from using that "feature" because runtime checks for bad optional dereference will kick when used (For instance, "nagfor -C=present" will produce an executable that abort with an error message . Flang does not have such runtime check option so far).


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

2 Files Affected:

  • (modified) flang/lib/Optimizer/CodeGen/CodeGen.cpp (+1-13)
  • (modified) flang/test/Fir/optional.fir (+1-1)
diff --git a/flang/lib/Optimizer/CodeGen/CodeGen.cpp b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
index 9b624efa053813..68b8c6613585e6 100644
--- a/flang/lib/Optimizer/CodeGen/CodeGen.cpp
+++ b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
@@ -3373,19 +3373,7 @@ struct AbsentOpConversion : public fir::FIROpConversion<fir::AbsentOp> {
   matchAndRewrite(fir::AbsentOp absent, OpAdaptor,
                   mlir::ConversionPatternRewriter &rewriter) const override {
     mlir::Type ty = convertType(absent.getType());
-    mlir::Location loc = absent.getLoc();
-
-    if (mlir::isa<fir::BoxCharType>(absent.getType())) {
-      auto structTy = mlir::cast<mlir::LLVM::LLVMStructType>(ty);
-      assert(!structTy.isOpaque() && !structTy.getBody().empty());
-      auto undefStruct = rewriter.create<mlir::LLVM::UndefOp>(loc, ty);
-      auto nullField =
-          rewriter.create<mlir::LLVM::ZeroOp>(loc, structTy.getBody()[0]);
-      rewriter.replaceOpWithNewOp<mlir::LLVM::InsertValueOp>(
-          absent, undefStruct, nullField, 0);
-    } else {
-      rewriter.replaceOpWithNewOp<mlir::LLVM::ZeroOp>(absent, ty);
-    }
+    rewriter.replaceOpWithNewOp<mlir::LLVM::ZeroOp>(absent, ty);
     return mlir::success();
   }
 };
diff --git a/flang/test/Fir/optional.fir b/flang/test/Fir/optional.fir
index 3b350d6fa94195..bded8b5332a300 100644
--- a/flang/test/Fir/optional.fir
+++ b/flang/test/Fir/optional.fir
@@ -47,7 +47,7 @@ func.func @foo3(%arg0: !fir.boxchar<1>) -> i1 {
 // CHECK-LABEL: @bar3
 func.func @bar3() -> i1 {
   %0 = fir.absent !fir.boxchar<1>
-  // CHECK: call i1 @foo3(ptr null, i64 undef)
+  // CHECK: call i1 @foo3(ptr null, i64 0)
   %1 = fir.call @foo3(%0) : (!fir.boxchar<1>) -> i1
   return %1 : i1
 }

Copy link
Contributor

@mjklemm mjklemm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jeanPerier jeanPerier merged commit 2f0b4f4 into llvm:main Oct 17, 2024
12 checks passed
@jeanPerier jeanPerier deleted the optional-concat branch October 17, 2024 11:25
@agozillon
Copy link
Contributor

Thank you very much for the quick fix @jeanPerier !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:codegen flang:fir-hlfir flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flang] Optional character argument used in string concatenation causes segfault
4 participants