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

[Tobias Gysi via flang-commits]

================
@@ -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>]>]> {
----------------
gysit wrote:

> I guess another option to "improve" these operations is not to mix the memory allocation effects with the effects on the stack pointer. What I mean by this:
> 
> - Any alloca will still have Alloc effect on AutomaticAllocationScopeResource.
> - Stacksave, stackrestore and all allocas will have read/write effects on AutomaticAllocationScopePointerResource non-addressable resource. This basically represents the fact that they read-modify the stack pointer.
> - Stackrestore in addition will have Free effect on AutomaticAllocationScopeResource.
> - The read/write effects on the non-addressable AutomaticAllocationScopePointerResource resource won't conflict with any loads/stores that access DefaultResource via a "pointer value", but they will guarantee proper ordering between stacksave, stackrestore and the allocas.

This sounds like a good way forward to me - as mentioned above - since it seems to properly reflect what these operations do under the hood.

> If you mean representing the allocation scopes with regions, then, yes, we have been discussing it several times, but did not take any actions due to its intrusivness.

Yep that is what I mean. I don't understand the requirements of Flang but it a region operation is a much nicer representation for such a scope like thing. We cannot do this in LLVM dialect since it needs to be close to LLVM IR and since we cannot really lift to such a more constraint representation during the import.

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