Skip to content
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

[flang][NFC] Use tablegen to reduce MemoryAllocationOpt boilerplate #90062

Merged
merged 2 commits into from
Apr 26, 2024

Conversation

tblah
Copy link
Contributor

@tblah tblah commented Apr 25, 2024

This is another one that runs on functions but isn't appropriate to also run on other top level operations. It needs to find all paths that return from the function to free heap allocated memory. There isn't a generic concept for general top level operations which is equivalent to looking for function returns.

I removed the manual definition of the options structure because there is already an identical definition in tablegen and the options are documented in Passes.td.

This is another one that runs on functions but isn't appropriate to also
run on other top level operations. It needs to find all paths that
return from the function to free heap allocated memory. There isn't a
generic concept for general top level operations which is equivalent to
looking for function returns.

I removed the manual definition of the options structure because there
is already an identical definition in tablegen and the options are
documented in Passes.td.
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Apr 25, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 25, 2024

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

Author: Tom Eccles (tblah)

Changes

This is another one that runs on functions but isn't appropriate to also run on other top level operations. It needs to find all paths that return from the function to free heap allocated memory. There isn't a generic concept for general top level operations which is equivalent to looking for function returns.

I removed the manual definition of the options structure because there is already an identical definition in tablegen and the options are documented in Passes.td.


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

4 Files Affected:

  • (modified) flang/include/flang/Optimizer/Transforms/Passes.h (-3)
  • (modified) flang/include/flang/Optimizer/Transforms/Passes.td (-1)
  • (modified) flang/include/flang/Tools/CLOptions.inc (+2-2)
  • (modified) flang/lib/Optimizer/Transforms/MemoryAllocation.cpp (+7-23)
diff --git a/flang/include/flang/Optimizer/Transforms/Passes.h b/flang/include/flang/Optimizer/Transforms/Passes.h
index fd7a4a3883c996..f3fd9c5bdbdbf4 100644
--- a/flang/include/flang/Optimizer/Transforms/Passes.h
+++ b/flang/include/flang/Optimizer/Transforms/Passes.h
@@ -57,15 +57,12 @@ std::unique_ptr<mlir::Pass>
 createExternalNameConversionPass(bool appendUnderscore);
 std::unique_ptr<mlir::Pass> createMemDataFlowOptPass();
 std::unique_ptr<mlir::Pass> createPromoteToAffinePass();
-std::unique_ptr<mlir::Pass> createMemoryAllocationPass();
 std::unique_ptr<mlir::Pass> createStackArraysPass();
 std::unique_ptr<mlir::Pass> createAliasTagsPass();
 std::unique_ptr<mlir::Pass>
 createAddDebugInfoPass(fir::AddDebugInfoOptions options = {});
 std::unique_ptr<mlir::Pass> createLoopVersioningPass();
 
-std::unique_ptr<mlir::Pass>
-createMemoryAllocationPass(bool dynOnHeap, std::size_t maxStackSize);
 std::unique_ptr<mlir::Pass> createAnnotateConstantOperandsPass();
 std::unique_ptr<mlir::Pass> createAlgebraicSimplificationPass();
 std::unique_ptr<mlir::Pass>
diff --git a/flang/include/flang/Optimizer/Transforms/Passes.td b/flang/include/flang/Optimizer/Transforms/Passes.td
index c3d5c336af40ba..77a36c8d8aaefc 100644
--- a/flang/include/flang/Optimizer/Transforms/Passes.td
+++ b/flang/include/flang/Optimizer/Transforms/Passes.td
@@ -247,7 +247,6 @@ def MemoryAllocationOpt : Pass<"memory-allocation-opt", "mlir::func::FuncOp"> {
            "std::size_t", /*default=*/"~static_cast<std::size_t>(0)",
            "Set maximum number of elements of an array allocated on the stack.">
   ];
-  let constructor = "::fir::createMemoryAllocationPass()";
 }
 
 def StackArrays : Pass<"stack-arrays", "mlir::ModuleOp"> {
diff --git a/flang/include/flang/Tools/CLOptions.inc b/flang/include/flang/Tools/CLOptions.inc
index f24716333d9acf..59014239f4753c 100644
--- a/flang/include/flang/Tools/CLOptions.inc
+++ b/flang/include/flang/Tools/CLOptions.inc
@@ -163,8 +163,8 @@ inline void addAVC(
 
 inline void addMemoryAllocationOpt(mlir::PassManager &pm) {
   addNestedPassConditionally<mlir::func::FuncOp>(pm, disableFirMao, [&]() {
-    return fir::createMemoryAllocationPass(
-        dynamicArrayStackToHeapAllocation, arrayStackAllocationThreshold);
+    return fir::createMemoryAllocationOpt(
+        {dynamicArrayStackToHeapAllocation, arrayStackAllocationThreshold});
   });
 }
 
diff --git a/flang/lib/Optimizer/Transforms/MemoryAllocation.cpp b/flang/lib/Optimizer/Transforms/MemoryAllocation.cpp
index 166a6b10def293..40b452a6202b07 100644
--- a/flang/lib/Optimizer/Transforms/MemoryAllocation.cpp
+++ b/flang/lib/Optimizer/Transforms/MemoryAllocation.cpp
@@ -28,17 +28,6 @@ namespace fir {
 static constexpr std::size_t unlimitedArraySize = ~static_cast<std::size_t>(0);
 
 namespace {
-struct MemoryAllocationOptions {
-  // Always move dynamic array allocations to the heap. This may result in more
-  // heap fragmentation, so may impact performance negatively.
-  bool dynamicArrayOnHeap = false;
-
-  // Number of elements in array threshold for moving to heap. In environments
-  // with limited stack size, moving large arrays to the heap can avoid running
-  // out of stack space.
-  std::size_t maxStackArraySize = unlimitedArraySize;
-};
-
 class ReturnAnalysis {
 public:
   MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(ReturnAnalysis)
@@ -68,8 +57,9 @@ class ReturnAnalysis {
 
 /// Return `true` if this allocation is to remain on the stack (`fir.alloca`).
 /// Otherwise the allocation should be moved to the heap (`fir.allocmem`).
-static inline bool keepStackAllocation(fir::AllocaOp alloca, mlir::Block *entry,
-                                       const MemoryAllocationOptions &options) {
+static inline bool
+keepStackAllocation(fir::AllocaOp alloca, mlir::Block *entry,
+                    const fir::MemoryAllocationOptOptions &options) {
   // Limitation: only arrays allocated on the stack in the entry block are
   // considered for now.
   // TODO: Generalize the algorithm and placement of the freemem nodes.
@@ -168,6 +158,9 @@ class MemoryAllocationOpt
     options = {dynOnHeap, maxStackSize};
   }
 
+  MemoryAllocationOpt(const fir::MemoryAllocationOptOptions &options)
+      : options{options} {}
+
   /// Override `options` if command-line options have been set.
   inline void useCommandLineOptions() {
     if (dynamicArrayOnHeap)
@@ -211,15 +204,6 @@ class MemoryAllocationOpt
   }
 
 private:
-  MemoryAllocationOptions options;
+  fir::MemoryAllocationOptOptions options;
 };
 } // namespace
-
-std::unique_ptr<mlir::Pass> fir::createMemoryAllocationPass() {
-  return std::make_unique<MemoryAllocationOpt>();
-}
-
-std::unique_ptr<mlir::Pass>
-fir::createMemoryAllocationPass(bool dynOnHeap, std::size_t maxStackSize) {
-  return std::make_unique<MemoryAllocationOpt>(dynOnHeap, maxStackSize);
-}

Copy link
Contributor

@clementval clementval left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the cleanup

Copy link
Contributor

@schweitzpgi schweitzpgi left a comment

Choose a reason for hiding this comment

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

Good to clean up some of this really old code and use "modern" tablegen features, etc. Thanks!

@tblah tblah merged commit 213ab96 into llvm:main Apr 26, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

4 participants