[Mlir-commits] [flang] [mlir] [mlir][LLVMIR] Set memory effects for llvm.intrin.stacksave/restore. (PR #191918)

[Slava Zakharin]

================
@@ -893,11 +893,36 @@ def LLVM_EhTypeidForOp : LLVM_OneResultIntrOp<"eh.typeid.for", [], [0]> {
 // Stack save/restore intrinsics.
 //
 
-def LLVM_StackSaveOp : LLVM_OneResultIntrOp<"stacksave", [0]> {
+// Technically, llvm.intrin.stacksave does not free the stack,
+// but MemFree<AutomaticAllocationScopeResource> seems to be
+// an appropriate effect to guarantee that the allocations
+// between stacksave/stackrestore cannot be moved out of
+// the stacksave/stackrestore "scope".
+// For example,
+//   %ptr = llvm.intrin.stacksave : !llvm.ptr
+//   %alloca = llvm.alloca %zero x i32 : (i32) -> !llvm.ptr
+//   ... = llvm.load %alloca : !llvm.ptr -> i32
+//   llvm.store %x, %alloca : i32, !llvm.ptr
+//   llvm.intrin.stackrestore %ptr : !llvm.ptr
+//
+// The 'llvm.alloca' should not be movable neither before
+// llvm.intrin.stacksave nor after llvm.intrin.stackrestore.
+// Moreover, llvm.intrin.stackrestore cannot be moved
+// above the load the the store.
+//
+// The 'Free' effect on 'AutomaticAllocationScopeResource'
+// for both stacksave and stackrestore should guarantee that.
+def LLVM_StackSaveOp
+    : LLVM_OneResultIntrOp<
+          "stacksave", [0], [],
+          [MemoryEffects<[MemFree<AutomaticAllocationScopeResource>]>]> {
----------------
vzakhari wrote:

Good question.

Here is my thinking. I am considering `AutomaticAllocationScopeResource` as a regular memory resource, so I think a `Read` should not prevent hoisting of `Alloc` above the `Read`. For example, a variable read should not prevent hoisting "a heap allocation" above it.

If we want to think of the operations that affect `AutomaticAllocationScopeResource` as affecting the stack pointer, I think we will have to use the following effects:
* stacksave - reads `AutomaticAllocationScopeResource`
* stackrestore - reads/writes `AutomaticAllocationScopeResource`
* Any alloca - reads/writes `AutomaticAllocationScopeResource`

These should guarantee proper ordering, but I think they will constraint optimizations too much (as long as `reads/writes `AutomaticAllocationScopeResource` overlaps with `DefaultResource`).

What do you think?

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