[PATCH] D136285: Bad optimization with alloca and intrinsic function stackrestore

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

In D136285#3889698 <https://reviews.llvm.org/D136285#3889698>, @jamieschmeiser wrote:

> I have verified that adding inalloca and removing tail call on stackrestore in my sample IR will fix the problem.

`inalloca` is intended to be used in conjunction with the `inalloca` argument attribute. It will make your allocas dynamic, as you say, but it's not designed for this purpose, and maybe that's OK, perhaps it should evolve into a "dynamic alloca" marker so we can be more intentional about static and dynamic allocas.



================
Comment at: llvm/test/Transforms/MemCpyOpt/stackrestore.ll:97
+  %SS = tail call ptr @llvm.stacksave()
+  %A2 = alloca [56 x i8], align 4
+  store i8 1, ptr %A2, align 4
----------------
jamieschmeiser wrote:
> rnk wrote:
> > My expectation is that all the allocas here are static: They are part of the entry block, they will be part of the initial stack allocation, they will not be affected by stacksave/restore. This does mean that simplifycfg can extend the lifetime of a stack allocation, but to my knowledge, that's valid, the program should have the same observable behavior.
> simplifycfg extending the lifetime of a stack allocation does have observable behaviour changes.  As pointed out in previous comments, it seems that stackrestore should not have tail on it, so consider the test IR without tail call on stackrestore and the code not in the entry block.
> 
> ```
> define dso_local signext i32 @test() {
> associate006_entry:
>   br label %b1
> 
> b1:
>   %A1 = alloca [56 x i8], align 8
>   %SS = tail call ptr @llvm.stacksave()
>   %A2 = alloca [56 x i8], align 4
>   store i8 1, ptr %A2, align 4
>   %GEP1 = getelementptr inbounds i8, ptr %A2, i32 8
>   store i8 1, ptr %GEP1, align 4
>   %GEP2 = getelementptr inbounds i8, ptr %A2, i32 12
>   store i8 1, ptr %GEP2, align 4
>   call void @llvm.memcpy.p0.p0.i32(ptr noundef nonnull align 8 dereferenceable(56) %A1, ptr noundef nonnull align 4 dereferenceable(56) %A2, i32 56, i1 false)
>   call void @llvm.stackrestore(ptr %SS)
>   %A3 = alloca [56 x i8], align 4
>   %uglygep123 = getelementptr i8, ptr %A3, i32 0
>   call void @llvm.memcpy.p0.p0.i32(ptr noundef nonnull align 1 dereferenceable(56) %uglygep123, ptr noundef nonnull align 8 dereferenceable(56) %A1, i32 56, i1 false)
>   ret i32 0
> }
> ```
> This will not pass the isStaticAlloca query and (due to the removed tail call) on stackrestore, no optimization will be performed by memcpyopt.  If simplifycfg is performed first, the two blocks are collapsed, this becomes the entry block and memcpyopt will do the optimization, changing the observable behaviour.
As long as the lifetimes of A1, A2, and A3 all last for the entire stack, I don't see any problems with the transform. From what I can tell using llc, they are all static allocas and the result of the program is the same.

I think you are right that stackrestore should not have a tail call marker on it, but that doesn't have any bearing on whether these are static or dynamic allocas.

Maybe this test is overreduced. Can you say more about why this transform is causing problems?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136285/new/

https://reviews.llvm.org/D136285