[PATCH] D140972: [flang] Add -fstack-arrays flag

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.
Herald added a subscriber: sunshaoce.


================
Comment at: clang/include/clang/Driver/Options.td:5056-5060
+def fstack_arrays : Flag<["-"], "fstack-arrays">, Group<f_Group>,
+  HelpText<"Attempt to allocate array temporaries on the stack, no matter their size">;
+def fno_stack_arrays : Flag<["-"], "fno-stack-arrays">, Group<f_Group>,
+  HelpText<"Allocate array temporaries on the heap (default)">;
+
----------------
We should avoid duplicating options like this and use multiclasses instead. For example, see how [[ https://github.com/llvm/llvm-project/blob/b6ceadf3b663427f3cc233bbfdb5e35017cabd9e/clang/include/clang/Driver/Options.td#L6464-L6467 | debug_pass_manager ]] is defined.


================
Comment at: flang/docs/FlangDriver.md:573
+`-O3 -ffast-math -fstack-arrays -fno-semantic-interposition`.
 `-fno-semantic-interposition` is not used because clang does not enable this as
 part of `-Ofast` as the default behaviour is similar.
----------------
[nit] "Clang" is a sub-project. It's not clear what "clang" would be - also a sub-project or the binary?


================
Comment at: flang/include/flang/Tools/CLOptions.inc:159
 inline void createDefaultFIROptimizerPassPipeline(
-    mlir::PassManager &pm, llvm::OptimizationLevel optLevel = defaultOptLevel) {
+    mlir::PassManager &pm, bool stackArrays = false, llvm::OptimizationLevel optLevel = defaultOptLevel) {
   // simplify the IR
----------------
CLANG-FORMAT-ME :) Same for other long lines here.


================
Comment at: flang/include/flang/Tools/CLOptions.inc:216
 inline void createMLIRToLLVMPassPipeline(
-    mlir::PassManager &pm, llvm::OptimizationLevel optLevel = defaultOptLevel) {
+    mlir::PassManager &pm, bool stackArrays = false, llvm::OptimizationLevel optLevel = defaultOptLevel) {
   // Add default optimizer pass pipeline.
----------------
[nit] To me, it would make more sense to put `stackArrays` at the end. `optLevel`is a more powerful flag. 


================
Comment at: flang/test/Transforms/stack-arrays.f90:3
 
+! We have to check llvm ir here to force flang to run the whole mlir pipeline
+! this is just to check that -fstack-arrays enables the stack-arrays pass so
----------------
Also, I don't quite follow this comment:

>  We have to check llvm ir here to force flang to run the whole mlir pipeline

Why is checking LLVM IR going to force Flang to run anything?


================
Comment at: flang/test/Transforms/stack-arrays.f90:6
+! only check the first example
+! RUN: %flang_fc1 -emit-llvm -o - -fstack-arrays %s | FileCheck --check-prefix=CHECK-LLVM %s
+
----------------



================
Comment at: flang/tools/bbc/bbc.cpp:276
     // Add O2 optimizer pass pipeline.
-    fir::createDefaultFIROptimizerPassPipeline(pm, llvm::OptimizationLevel::O2);
+    fir::createDefaultFIROptimizerPassPipeline(pm, false,
+                                               llvm::OptimizationLevel::O2);
----------------
Similar suggestion below.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140972/new/

https://reviews.llvm.org/D140972