[flang-commits] [flang] [flang] Fix stack-arrays pass moving alloca across stackrestore scope (PR #184727)

[via flang-commits]

================
@@ -668,6 +668,25 @@ InsertionPoint AllocMemConversion::findAllocaInsertionPoint(
     // there were value operands to the allocmem so insert after the last one
     LLVM_DEBUG(llvm::dbgs()
                << "--Placing after last operand: " << *lastOperand << "\n");
+    // Check we aren't moving across a stackrestore scope boundary.
+    // The last operand may have been defined in an earlier stacksave/
+    // stackrestore scope (e.g. due to CSE merging identical size computations,
+    // or HLFIR-to-FIR lowering reusing a single size value across scopes).
+    // Placing the alloca at the operand's location would put it in the wrong
+    // scope, where it gets reclaimed before the allocmem's actual use.
+    // Fall back to the allocmem's own location, matching the "operand declared
+    // in a different block" bail-out logic above.
+    if (lastOperand->getBlock() == oldAlloc->getBlock()) {
----------------
jeanPerier wrote:

Why restrict the check to cases where the new insertion point (last operand) is in the same block as the old alloca?

The generic check should be to visit all operations in between, regardless of whether there are in the same block (case can be made that there is no need to visit regions of operations in between because a stack_restore inside a region should always be paired with a stack_save, otherwise there is a big scoping issue in the IR).

If this visit cannot easily be made (block CFG is too complex), the code should bail to using the oldAlloc as insertion point.

Here are two examples:

The first is derived from your example by adding an if to nest the oldAlloc in a block inside a region for an if operation belonging to the same block as the lastOperand. It will also lead to a bug with the current pipeline even after your fix:

```
subroutine ss1(a, l)
  character*(*) a(1)
  character*1 t_s
  logical :: l
  call u(t_s() // '?' // a)
  if (l) then
    call u(t_s() // '?' // a)
  end if
end subroutine
```

The second example is more tricky as it involve block CFG (there is still a single region). It cannot be obtained with the current default pipeline, but as we add more optimizations, we should still consider it a possibility. You can the invalid IR after `flang -fc1 -O2 -emit-fir -o - repro.f90 | fir-opt -mem2reg --inline -inline-all -cse -stack-arrays`.

```
module awful       
contains       
  
subroutine trouble(n)
  integer, value :: n      
  character(1) :: func, c
  ! stack save/restore created around something that will be inlined.
  call something(n, func())  
  call create_temp([(i, i=1,n+1)])
end subroutine
                                                               
                                                               
subroutine something(n, c)                                     
  integer, value :: n                                          
  logical :: l                                                 
  character(1) :: c
  ! dominating  computation of `n+1` where (lastOperand insertion point after inlining) 
  call create_temp([(i, i=1,n+1)]) 
  ! block CFG created inside  something                  
  if (l) go to 1                                               
2  call bazz()                                                 
   go to 3                                                     
1  call buzz()                                                 
   go to 2                                                     
3  continue                                                    
end subroutine                                                 
end module   
```

https://github.com/llvm/llvm-project/pull/184727