[PATCH] D118538: [SLP] Schedule only sub-graph of vectorizable instructions

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Well, it seems I got slightly ahead of myself.  When reviewing the diff before push, I spotted another questionable looking alloca/stacksave reordering.  We appear to have one more missing dependency.

define void @stacksave2(i8** %a, i8** %b, i8** %c) {
 ; CHECK-LABEL: @stacksave2(
-; CHECK-NEXT:    [[V1:%.*]] = alloca i8, align 1
 ; CHECK-NEXT:    [[STACK:%.*]] = call i8* @llvm.stacksave()
+; CHECK-NEXT:    [[B2 <https://reviews.llvm.org/B2>:%.*]] = getelementptr i8*, i8** [[B:%.*]], i32 1
+; CHECK-NEXT:    [[V1:%.*]] = alloca i8, align 1

Here we are sinking an alloca from above a stacksave into the region of the save/restore.  This looks pretty suspect as there could be a use of v1 after the stackrestore and thus the transform could introduce an out of bounds access on the stack.   The only way I can see this being correct was if the original program would have to be undefined, but I don't currently why that would be the case.

Here's where I'm very happy I spent time to write extensive tests even though at the time I didn't think we needed this dependence.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118538