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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
