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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.


================
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
----------------
tblah wrote:
> awarzynski wrote:
> > CLANG-FORMAT-ME :) Same for other long lines here.
> It seems clang-format is not run on this file. I have fixed the lines changed in my patch. Should I clang-format the whole file in a separate patch?
+1


================
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
----------------
tblah wrote:
> awarzynski wrote:
> > 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?
> Just running `flang-new -fc1 -emit-fir` outputs the FIR before all of the transformation passes run (which is why I have to pipe the result through `fir-opt` to run the array value copy and stack arrays passes on the first line).
> 
> Outputting LLVM IR requires all of the MLIR transformation passes to be run to transform all of the higher level dialects into the LLVM dialect. Stack arrays is run as part of that same pipeline (`fir::createMLIRToLLVMPassPipeline`). I want to test that `-fstack-arrays` does cause the stack arrays pass to run as part of that pipeline, which is why I am checking the LLVM-IR.
> 
> One alternative would be to print out which passes have run and check that, but I felt this way fits more with the spirit of the other tests.
Thanks for the explanation. So you want to verify this functionality by investigating two pipelines:
* source --> FIR
* source --> LLVM IR
You could convey that by using `CHECK-FIR` and `CHECK-LLVM-IR` (or just `FIR` and `LLVM-IR`). I "complained", because this is unclear:
> We have to check llvm ir here to force flang to run the whole mlir pipeline
Perhaps:
> In order to verify the whole MLIR pipeline, make the driver generate LLVM IR.


================
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
+
----------------
tblah wrote:
> awarzynski wrote:
> > 
> Lots of other tests do not follow this convention. It would change the FileCheck comments to look like `! LLVM: array_value_copy_simple`. 
> 
> I think it is good to require `CHECK-FEATURE:` so that any mention of `FEATURE` in comments does not confuse FileCheck. Especially for a name like LLVM which is likely to be written in capitals.
> Lots of other tests do not follow this convention.

I think that we would easily find examples for either approach.

> I think it is good to require CHECK-FEATURE:

Well, `LLVM` is not a feature ;-) Also, most folks working with LLVM are familiar with the LIT syntax - the presence of `CHECK` in `CHECK-FEATURE` is just unnecessary noise to me. But I am happy either way. But I would appreciate replacing with `LLVM` with something a bit more meaningful - perhaps `LLVM-IR`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140972