Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LAA] Analyze pointers forked by a phi #65834

Merged
merged 1 commit into from
Sep 19, 2023
Merged

[LAA] Analyze pointers forked by a phi #65834

merged 1 commit into from
Sep 19, 2023

Conversation

vfdff
Copy link
Contributor

@vfdff vfdff commented Sep 9, 2023

Given a function like the following: https://godbolt.org/z/T9c99fr88

1161_noReadWrite(int *Preds) {
  for (int i = 0; i < LEN_1D-1; ++i) {
    if (Preds[i] != 0)
      b[i] = c[i] + 1;
    else
      a[i] = i * i;
  }
}

LLVM will optimize the IR to a single store by a phi instruction:

   %1 = load ptr, ptr @a, align 64
   %2 = load ptr, ptr @b, align 64
   ...
 for.inc:
   %.sink = phi ptr [ %1, %if.then ], [ %2, %if.else ]
   %add.sink = phi double [ %add, %if.then ], [ %conv8, %if.else ]
   %arrayidx7 = getelementptr inbounds double, ptr %.sink, i64 %indvars.iv
   store double %add.sink, ptr %arrayidx7, align 8

LAA is currently unable to analyze such IR, since ScalarEvolution will return a SCEVUnknown for the forked pointer operand of the store.

This patch adds initial optional support for analyzing both possibilities for the pointer and allowing LAA to generate runtime checks for the bounds if required, refers to D108699, but here address the phi node.

Fixes #64888

Given a function like the following: https://godbolt.org/z/T9c99fr88
    ```
    1161_noReadWrite(int *Preds) {
      for (int i = 0; i < LEN_1D-1; ++i) {
        if (Preds[i] != 0)
          b[i] = c[i] + 1;
        else
          a[i] = i * i;
      }
    }
    ```

LLVM will optimize the IR to a single store by a phi instruction:

   ```
      %1 = load ptr, ptr @A, align 64
      %2 = load ptr, ptr @b, align 64
      ...
    for.inc:
      %.sink = phi ptr [ %1, %if.then ], [ %2, %if.else ]
      %add.sink = phi double [ %add, %if.then ], [ %conv8, %if.else ]
      %arrayidx7 = getelementptr inbounds double, ptr %.sink, i64 %indvars.iv
      store double %add.sink, ptr %arrayidx7, align 8
   ```

LAA is currently unable to analyze such IR, since ScalarEvolution
will return a SCEVUnknown for the forked pointer operand of the store.

This patch adds initial optional support for analyzing both possibilities for the pointer
and allowing LAA to generate runtime checks for the bounds if required, refers to D108699, but here address the phi node.

Fixes llvm#64888
@llvmbot
Copy link
Member

llvmbot commented Sep 9, 2023

@llvm/pr-subscribers-llvm-analysis

Changes
diff --git a/llvm/lib/Analysis/LoopAccessAnalysis.cpp b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
index 4dd150492453f72..565cbfdc22d8466 100644
--- a/llvm/lib/Analysis/LoopAccessAnalysis.cpp
+++ b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
@@ -942,6 +942,22 @@ static void findForkedSCEVs(
       ScevList.emplace_back(Scev, !isGuaranteedNotToBeUndefOrPoison(Ptr));
     break;
   }
+  case Instruction::PHI: {
+    SmallVector, 2> ChildScevs;
+    // A phi means we've found a forked pointer, but we currently only
+    // support a single phi per pointer so if there's another behind this
+    // then we just bail out and return the generic SCEV.
+    if (I->getNumOperands() == 2) {
+      findForkedSCEVs(SE, L, I->getOperand(0), ChildScevs, Depth);
+      findForkedSCEVs(SE, L, I->getOperand(1), ChildScevs, Depth);
+    }
+    if (ChildScevs.size() == 2) {
+      ScevList.push_back(ChildScevs[0]);
+      ScevList.push_back(ChildScevs[1]);
+    } else
+      ScevList.emplace_back(Scev, !isGuaranteedNotToBeUndefOrPoison(Ptr));
+    break;
+  }
   case Instruction::Add:
   case Instruction::Sub: {
     SmallVector> LScevs;
diff --git a/llvm/test/Analysis/LoopAccessAnalysis/forked-pointers.ll b/llvm/test/Analysis/LoopAccessAnalysis/forked-pointers.ll
index 3f5c8e4c1c72ff1..30ffc4cb5e1e85f 100644
--- a/llvm/test/Analysis/LoopAccessAnalysis/forked-pointers.ll
+++ b/llvm/test/Analysis/LoopAccessAnalysis/forked-pointers.ll
@@ -936,3 +936,126 @@ for.body:
 exit:
   ret void
 }
+
+define void @forked_ptrs_with_different_base(ptr nocapture readonly %Preds, ptr nocapture %a, ptr nocapture %b, ptr nocapture readonly %c)  {
+; CHECK:       for.body:
+; CHECK-NEXT:    Memory dependences are safe with run-time checks
+; CHECK-NEXT:    Dependences:
+; CHECK-NEXT:    Run-time memory checks:
+; CHECK-NEXT:    Check 0:
+; CHECK-NEXT:      Comparing group ([[G1:.+]]):
+; CHECK-NEXT:        %arrayidx7 = getelementptr inbounds double, ptr %.sink, i64 %indvars.iv
+; CHECK-NEXT:      Against group ([[G2:.+]]):
+; CHECK-NEXT:        %arrayidx = getelementptr inbounds i32, ptr %Preds, i64 %indvars.iv
+; CHECK-NEXT:    Check 1:
+; CHECK-NEXT:      Comparing group ([[G1]]):
+; CHECK-NEXT:        %arrayidx7 = getelementptr inbounds double, ptr %.sink, i64 %indvars.iv
+; CHECK-NEXT:      Against group ([[G4:.+]]):
+; CHECK-NEXT:        %arrayidx5 = getelementptr inbounds double, ptr %0, i64 %indvars.iv
+; CHECK-NEXT:    Check 2:
+; CHECK-NEXT:      Comparing group ([[G3:.+]]):
+; CHECK-NEXT:        %arrayidx7 = getelementptr inbounds double, ptr %.sink, i64 %indvars.iv
+; CHECK-NEXT:      Against group ([[G2]]):
+; CHECK-NEXT:        %arrayidx = getelementptr inbounds i32, ptr %Preds, i64 %indvars.iv
+; CHECK-NEXT:    Check 3:
+; CHECK-NEXT:      Comparing group ([[G3]]):
+; CHECK-NEXT:        %arrayidx7 = getelementptr inbounds double, ptr %.sink, i64 %indvars.iv
+; CHECK-NEXT:      Against group ([[G4]]):
+; CHECK-NEXT:        %arrayidx5 = getelementptr inbounds double, ptr %0, i64 %indvars.iv
+; CHECK-NEXT:    Grouped accesses:
+; CHECK-NEXT:      Group [[G1]]:
+; CHECK-NEXT:        (Low: %1 High: (63992 + %1))
+; CHECK-NEXT:          Member: {%1,+,8}<%for.body>
+; CHECK-NEXT:      Group [[G3]]:
+; CHECK-NEXT:        (Low: %2 High: (63992 + %2))
+; CHECK-NEXT:          Member: {%2,+,8}<%for.body>
+; CHECK-NEXT:      Group [[G2]]:
+; CHECK-NEXT:        (Low: %Preds High: (31996 + %Preds))
+; CHECK-NEXT:          Member: {%Preds,+,4}<%for.body>
+; CHECK-NEXT:      Group [[G4]]:
+; CHECK-NEXT:        (Low: %0 High: (63992 + %0))
+; CHECK-NEXT:          Member: {%0,+,8}<%for.body>
+; CHECK-EMPTY:
+; CHECK-NEXT:    Non vectorizable stores to invariant address were not found in loop.
+entry:
+  %0 = load ptr, ptr %c, align 64
+  %1 = load ptr, ptr %a, align 64
+  %2 = load ptr, ptr %b, align 64
+  br label %for.body
+
+for.cond.cleanup:                                 ; preds = %for.inc
+  ret void
+
+for.body:                                         ; preds = %entry, %for.inc
+  %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.inc ]
+  %arrayidx = getelementptr inbounds i32, ptr %Preds, i64 %indvars.iv
+  %3 = load i32, ptr %arrayidx, align 4
+  %cmp2.not = icmp eq i32 %3, 0
+  br i1 %cmp2.not, label %if.else, label %if.then
+
+if.then:                                          ; preds = %for.body
+  %arrayidx5 = getelementptr inbounds double, ptr %0, i64 %indvars.iv
+  %4 = load double, ptr %arrayidx5, align 8
+  %add = fadd fast double %4, 1.000000e+00
+  br label %for.inc
+
+if.else:                                          ; preds = %for.body
+  %5 = mul nuw nsw i64 %indvars.iv, %indvars.iv
+  %6 = trunc i64 %5 to i32
+  %conv8 = sitofp i32 %6 to double
+  br label %for.inc
+
+for.inc:                                          ; preds = %if.then, %if.else
+  %.sink = phi ptr [ %1, %if.then ], [ %2, %if.else ]
+  %add.sink = phi double [ %add, %if.then ], [ %conv8, %if.else ]
+  %arrayidx7 = getelementptr inbounds double, ptr %.sink, i64 %indvars.iv
+  store double %add.sink, ptr %arrayidx7, align 8
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
+  %exitcond.not = icmp eq i64 %indvars.iv.next, 7999
+  br i1 %exitcond.not, label %for.cond.cleanup, label %for.body
+}
+
+; Negative test: the operator number of PhiNode is not 2.
+define void @forked_ptrs_with_different_base3(ptr nocapture readonly %Preds, ptr nocapture %a, ptr nocapture %b, ptr nocapture readonly %c)  {
+; CHECK:       for.body:
+; CHECK-NEXT:    Report: cannot identify array bounds
+; CHECK-NEXT:    Dependences:
+; CHECK-NEXT:    Run-time memory checks:
+; CHECK-NEXT:    Grouped accesses:
+; CHECK-EMPTY:
+; CHECK-NEXT:    Non vectorizable stores to invariant address were not found in loop.
+entry:
+  %ld.c = load ptr, ptr %c, align 64
+  %ld.a = load ptr, ptr %a, align 64
+  %ld.b = load ptr, ptr %b, align 64
+  br label %for.body
+
+for.body:                                         ; preds = %entry, %for.inc
+  %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.inc ]
+  %arrayidx = getelementptr inbounds i32, ptr %Preds, i64 %indvars.iv
+  %ld.preds = load i32, ptr %arrayidx, align 4
+  switch i32 %ld.preds, label %if.else [
+    i32 0, label %if.br0
+	i32 1, label %if.br1
+  ]
+
+if.br0:                                          ; preds = %for.body
+  br label %for.inc
+
+if.br1:                                          ; preds = %for.body
+  br label %for.inc
+
+if.else:                                         ; preds = %for.body
+  br label %for.inc
+
+for.inc:                                          ; preds = %if.br1, %if.br0
+  %.sink = phi ptr [ %ld.a, %if.br0 ], [ %ld.b, %if.br1 ], [ %ld.c, %if.else ]
+  %arrayidx7 = getelementptr inbounds double, ptr %.sink, i64 %indvars.iv
+  store double 1.000000e+00, ptr %arrayidx7, align 8
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
+  %exitcond.not = icmp eq i64 %indvars.iv.next, 7999
+  br i1 %exitcond.not, label %for.cond.cleanup, label %for.body
+
+for.cond.cleanup:                                 ; preds = %for.inc
+  ret void
+}
\ No newline at end of file

br label %for.inc

for.inc: ; preds = %if.then, %if.else
%.sink = phi ptr [ %1, %if.then ], [ %2, %if.else ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @vfdff, this test doesn't seem to match the actual C code example given in the commit message. In this test the pointers are loop invariant and defined outside the loop, whereas in your C example we store to a different location in each iteration, i.e. something like

  if (cond[i])
    b[i] = a[i] * a[i];
  else
    c[i] = i + 3;
}

I think it's worth writing a test for a PHI node with varying pointer values to make sure it does what you expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @david-arm , thanks for your comment.
Yes, the IR is not exactly match the C example, but I only adjust the global variables to the argument.
here is its C example ,https://godbolt.org/z/nn6eh5dK4, I think in this IR, the store pointers is not a loop invariant because its offset %indvars.iv will change in each iteration.

Copy link
Collaborator

@huntergr-arm huntergr-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vfdff
Copy link
Contributor Author

vfdff commented Sep 19, 2023

hi @huntergr-arm, thanks very much for the review.

@vfdff vfdff merged commit 48caa07 into llvm:main Sep 19, 2023
ZijunZhaoCCK pushed a commit to ZijunZhaoCCK/llvm-project that referenced this pull request Sep 19, 2023
Given a function like the following: https://godbolt.org/z/T9c99fr88
```c
1161_noReadWrite(int *Preds) {
  for (int i = 0; i < LEN_1D-1; ++i) {
    if (Preds[i] != 0)
      b[i] = c[i] + 1;
    else
      a[i] = i * i;
  }
}
```

LLVM will optimize the IR to a single store by a phi instruction:

   ```llvm
      %1 = load ptr, ptr @A, align 64
      %2 = load ptr, ptr @b, align 64
      ...
    for.inc:
      %.sink = phi ptr [ %1, %if.then ], [ %2, %if.else ]
      %add.sink = phi double [ %add, %if.then ], [ %conv8, %if.else ]
%arrayidx7 = getelementptr inbounds double, ptr %.sink, i64 %indvars.iv
      store double %add.sink, ptr %arrayidx7, align 8
   ```

LAA is currently unable to analyze such IR, since ScalarEvolution will
return a SCEVUnknown for the forked pointer operand of the store.

This patch adds initial optional support for analyzing both
possibilities for the pointer and allowing LAA to generate runtime
checks for the bounds if required, refers to D108699, but here address
the phi node.

Fixes llvm#64888

Reviewed By: huntergr-arm, fhahn
Differential Revision: https://reviews.llvm.org/D158965
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vectorization] support forked pointer from loop body with control flow
5 participants