In D140415#4080080, @jdoerfert wrote:

Did you check LLVM's heap2stack and the corresponding tests?
https://github.com/llvm/llvm-project/blob/c68af565ff0c2fdc5537e9ac0c2d7c75df44b035/llvm/lib/Transforms/IPO/AttributorAttributes.cpp#L6480
https://github.com/llvm/llvm-project/blob/main/llvm/test/Transforms/Attributor/heap_to_stack.ll
https://github.com/llvm/llvm-project/blob/main/llvm/test/Transforms/Attributor/heap_to_stack_gpu.ll

In D140415#4080080, @jdoerfert wrote:

Thanks for taking a look, see my responses inline. For more information, the RFC is at https://reviews.llvm.org/D139617

• Add missing early return for allocations not for arrays
• Remove braces from if statement with a single statement in its body
• Assert that a correct insertion point is found for the alloca

• Check for a parent implementing LoopLikeOpInterface before searching for a CFG loop
• Don't use recursion when searching for CFG cycles
• Stop hierarchical search at anything implementing FunctionOpInterface, not just func.func

Ping for review

Ping for any MLIR reviewers

• Rename option macro to make it clear that it doesn't do marshalling

Fix newly added tests

Update to ensure that generated code for Options.td does not try to reference
clang::CodegenOpts::StackArrays.

Updated getSourceRank and getDestRank to return size_t to avoid comparison
between int64_t and size_t.

Handle case where blocks are nested inside of eachother

Clarified a comment in stack-arrays.f90 and clarified the choice of FileCheck
prefix.

Updated a comment and documentation string in the test pass to make it clear
the loop detection is no longer a method in mlir::Block

@rriddle ping

• Implement operator!= as !(operator==)
• Move insertion point computation to StackArraysAnalysisWrapper::analyseFunction
• Remove special-casing for join(allocated, unknown)
• Add processing of fir.result to AllocationAnalysis::visitOperation so that lattices are propagated out of nested blocks
• Walk function returns not freememops
• Add a test checking that the data flow analysis gives correct results for scf.if

Update to address review comments

Updating patch context

Updated to construct StackArrays as a ModuleOp pass

• Do not move allocations outside of openmp regions
• Detect loops in the control flow graph
• Attempt to use llvm.stacksave/llvm.stackrestore to allow stack allocations inside of loops

Thanks for review @DavidTruby

Updated to use llvm::any_of instead of a for loop and an if.

• Add mention of CFG loops, removed TODO for goto statements
• Specified the conditions under which llvm.stacksave/llvm.stackrestore intrinsics are safe to use

Moved to a stand alone function in LoopLikeInterface.h

In D140415#4030299, @peixin wrote:

In D140415#4028439, @tblah wrote:

In D140415#4026250, @jeanPerier wrote:

Maybe it's worth testing SPEC 2017 (and SNAP?) about the compilation time increase and the performance improvement with -fstack-arrays?

In D140415#4026250, @jeanPerier wrote:

I am wondering about its cost compared to a solution that would apply the option when creating the allocations in lowering in a centralized FirOpBuilder helper to create array temps. Do you have any idea of the complexity of this pass? e.g. is it linear with the number of blocks/FIR instruction?

• Remove quote from gfortran's man page
• Fix typo

Update patch again because I missed a test removal in the last update

Un-WIP'ed, removed tests for the two unsupported cases (see the RFC for more information).

• Added a paragraph explaining handling of fir.call operations during data flow analysis
• Added information about the two known cases where the existing analysis is insufficient (see the section on ConvertExpr.cpp)

In D140879#4023248, @Mogball wrote:

+1 but why is the rename necessary? From what I understand, the name shouldn't conflict with any other method...

Changes in the new patch version

• Calculate allocmem insertion point in the analysis stage so that we can catch all failures during analysis and then succeed when making real code changes
• Pass the lattice through fir.call operations. This is important so that we move allocations for array temporaries created for function arguments. It is safe to assume that functions do not change the allocation status of the allocations we are tracking because we only track allocations for temporaries created by flang.
• Add some tests. The last three tests in stack-arrays.f90 don't check anything useful yet: they are placeholders for when the stack arrays can handle these cases correctly.

Thanks for review.

There is a WIP implementation of the RFC at https://reviews.llvm.org/D140415

Add a section naming a new FIR attribute to mark allocations which should never be moved to the stack: fir.must_be_heap

Thank you everyone for the feedback so far.

In D138675#3995282, @probinson wrote:

See D139967 for why UNSUPPORTED: powerpc didn't work. That patch will put it back, and also update the lit config so the check will work now.

LGTM, thanks for fixing this!

In D138675#3989826, @amyk wrote:

Thanks for the follow up patch! I tested the patch locally and also saw the buildbot results, and it doesn't appear like the follow up patch marked powerpc as unsupported as the error still persists (https://lab.llvm.org/buildbot/#/builders/21/builds/57850).
I was playing around with the test case locally and what appears to work is doing something like:

! UNSUPPORTED: powerpc-registered-target

In D138675#3989403, @amyk wrote:

Hi,

The ppc64le-flang-rhel-clang bot is also experiencing some failures with the Driver/fast_math.f90 test case. In particular, crtfastmath.o is not found at all for CHECK-CRT as we can see in the failure details.

llvm-project/flang/test/Driver/fast_math.f90:64:19: error: CHECK-CRT-SAME: expected string not found in input
! CHECK-CRT-SAME: crtfastmath.o

The output for the CHECK-CRT line on the bot looks like the following:

"/home/buildbots/ppc64le-flang-rhel-test/ppc64le-flang-rhel-clang-build/build/bin/flang-new" "-fc1" "-triple" "powerpc64le-unknown-linux-gnu" "-emit-obj" "-mrelocation-model" "pic" "-pic-level" "2" "-pic-is-pie" "-ffast-math" "-target-cpu" "ppc64le" "-o" "/tmp/lit-tmp-x_okwzug/fast_math-e9ab49.o" "-x" "f95-cpp-input" "/home/buildbots/ppc64le-flang-rhel-test/ppc64le-flang-rhel-clang-build/llvm-project/flang/test/Driver/fast_math.f90" 

"/usr/bin/ld" "-pie" "--hash-style=gnu" "--eh-frame-hdr" "-m" "elf64lppc" "-dynamic-linker" "/lib64/ld64.so.2" "-o" "/home/buildbots/ppc64le-flang-rhel-test/ppc64le-flang-rhel-clang-build/build/tools/flang/test/Driver/Output/fast_math.f90.tmp" "/usr/lib/gcc/ppc64le-redhat-linux/8/../../../../lib64/Scrt1.o" "/usr/lib/gcc/ppc64le-redhat-linux/8/../../../../lib64/crti.o" "/usr/lib/gcc/ppc64le-redhat-linux/8/crtbeginS.o" "-L/usr/lib/gcc/ppc64le-redhat-linux/8" "-L/usr/lib/gcc/ppc64le-redhat-linux/8/../../../../lib64" "-L/lib/../lib64" "-L/usr/lib/../lib64" "-L/lib" "-L/usr/lib" "/tmp/lit-tmp-x_okwzug/fast_math-e9ab49.o" "-lFortran_main" "-lFortranRuntime" "-lFortranDecimal" "-lm" "-lgcc" "--as-needed" "-lgcc_s" "--no-as-needed" "-lc" "-lgcc" "--as-needed" "-lgcc_s" "--no-as-needed" "/usr/lib/gcc/ppc64le-redhat-linux/8/crtendS.o" "/usr/lib/gcc/ppc64le-redhat-linux/8/../../../../lib64/crtn.o"

Would it be possible to follow up with a fix for this test case?

In D138675#3987836, @clementval wrote:

I have the Driver/fast_math.f90 failing on my side.

llvm-project/flang/test/Driver/fast_math.f90:63:14: error: CHECK-CRT: expected string not found in input
! CHECK-CRT: crtbeginS.o

I have a crtbegin.o in the link line but not a crtbeginS.o

ping @nicolasvasilache

Update with nitpicks to documentation (mostly to re-run Windows CI)

Failed Tests (5):

Flang :: Fir/boxproc.fir
Flang :: Fir/target-rewrite-arg-position.fir
Flang :: Fir/target-rewrite-boxchar.fir
Flang :: Fir/target-rewrite-char-proc.fir
Flang :: Fir/target.fir

Update patch to skip the test for linking to crtfastmath.o on Windows as there doesn't seem to be a Windows equivalent of crtfastmath.o.

In D139617#3981121, @clementval wrote:

In D139617#3981099, @kiranchandramohan wrote:

Could you also comment on why a general pass like MemoryAllocationOpt that converts Stack Allocation to Heap Allocation is not suitable here? Please include this in the RFC as an alternative option that was considered.

That would be good to know. Modifying the behavior in all the mentioned places sounds a bit hacky and error prone in the future. How are you going to ensure future code will follow what you propose here. I think a centralized pass is preferred even if it requires extra attributes or smth similar.

• Organize document according to the format recommended in DesignGuideline.md
  • Explain why a piecemeal approach was chosen instead of a single pass performing this conversion
  • Mention that most of the referenced code will be replaced by HLFIR
  • Include an example of the changes to array value copy
  • Provide a more detailed explanation of ConvertVariable.cpp
  • Add a testing plan

Updating patch add documentation of the meaning of -Ofast, -ffast-math, and comparison with gfortran and nvfortran.

Thanks for this! LGTM

In D139322#3970772, @nicolasvasilache wrote:

commit message seems hosed

Updated comment, updated patch context, fixed unused variable

Sorry about this, it should be fixed by https://github.com/llvm/llvm-project/commit/3a1b40925486c1eea80e2a0e636b208435487daf

Add test for -fstack-arrays usage warning