[llvm] ee94a4a - [Attributor][FIX] Avoid endless recursion, simple case

[Johannes Doerfert via llvm-commits]

Author: Johannes Doerfert
Date: 2022-03-23T15:55:32-05:00
New Revision: ee94a4a3d02f0cc7496eb91ea7d5c0819a6b32a0

URL: https://github.com/llvm/llvm-project/commit/ee94a4a3d02f0cc7496eb91ea7d5c0819a6b32a0
DIFF: https://github.com/llvm/llvm-project/commit/ee94a4a3d02f0cc7496eb91ea7d5c0819a6b32a0.diff

LOG: [Attributor][FIX] Avoid endless recursion, simple case

There is potential for endless recursion if we try to determine the
underlying objects of a load, just to end up with the load as underlying
object. A proper solution will require us to pass a visited set around.
This will happen as we cleanup genericValueTraversal soon.

Added: 
    llvm/test/Transforms/OpenMP/attributor_recursion_crash.ll

Modified: 
    llvm/lib/Transforms/IPO/AttributorAttributes.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
index 1a6634822cd8f..b6bb332503e8d 100644
--- a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
+++ b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
@@ -403,25 +403,29 @@ static bool genericValueTraversal(
 
     if (auto *LI = dyn_cast<LoadInst>(V)) {
       bool UsedAssumedInformation = false;
-      SmallSetVector<Value *, 4> PotentialCopies;
-      if (AA::getPotentiallyLoadedValues(A, *LI, PotentialCopies, QueryingAA,
-                                         UsedAssumedInformation,
-                                         /* OnlyExact */ true)) {
-        // Values have to be dynamically unique or we loose the fact that a
-        // single llvm::Value might represent two runtime values (e.g., stack
-        // locations in 
diff erent recursive calls).
-        bool DynamicallyUnique =
-            llvm::all_of(PotentialCopies, [&A, &QueryingAA](Value *PC) {
-              return AA::isDynamicallyUnique(A, QueryingAA, *PC);
-            });
-        if (DynamicallyUnique &&
-            (!Intraprocedural || !CtxI ||
-             llvm::all_of(PotentialCopies, [CtxI](Value *PC) {
-               return AA::isValidInScope(*PC, CtxI->getFunction());
-             }))) {
-          for (auto *PotentialCopy : PotentialCopies)
-            Worklist.push_back({PotentialCopy, CtxI});
-          continue;
+      // If we ask for the potentially loaded values from the initial pointer we
+      // will simply end up here again. The load is as far as we can make it.
+      if (LI->getPointerOperand() != InitialV) {
+        SmallSetVector<Value *, 4> PotentialCopies;
+        if (AA::getPotentiallyLoadedValues(A, *LI, PotentialCopies, QueryingAA,
+                                           UsedAssumedInformation,
+                                           /* OnlyExact */ true)) {
+          // Values have to be dynamically unique or we loose the fact that a
+          // single llvm::Value might represent two runtime values (e.g., stack
+          // locations in 
diff erent recursive calls).
+          bool DynamicallyUnique =
+              llvm::all_of(PotentialCopies, [&A, &QueryingAA](Value *PC) {
+                return AA::isDynamicallyUnique(A, QueryingAA, *PC);
+              });
+          if (DynamicallyUnique &&
+              (!Intraprocedural || !CtxI ||
+               llvm::all_of(PotentialCopies, [CtxI](Value *PC) {
+                 return AA::isValidInScope(*PC, CtxI->getFunction());
+               }))) {
+            for (auto *PotentialCopy : PotentialCopies)
+              Worklist.push_back({PotentialCopy, CtxI});
+            continue;
+          }
         }
       }
     }

diff  --git a/llvm/test/Transforms/OpenMP/attributor_recursion_crash.ll b/llvm/test/Transforms/OpenMP/attributor_recursion_crash.ll
new file mode 100644
index 0000000000000..efc21c5169f95
--- /dev/null
+++ b/llvm/test/Transforms/OpenMP/attributor_recursion_crash.ll
@@ -0,0 +1,56 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --scrub-attributes
+; RUN: opt -passes=openmp-opt -S < %s | FileCheck %s
+; TODO: This should have a second test case with a chain load->phi->load->phi
+;                                                           A              |
+;                                                           \-------------/
+
+%"struct.TS" = type { i32, %"struct.TS"* }
+
+define weak amdgpu_kernel void @k() {
+; CHECK-LABEL: define {{[^@]+}}@k() {
+; CHECK-NEXT:  BB1:
+; CHECK-NEXT:    br label [[BB2:%.*]]
+; CHECK:       BB2:
+; CHECK-NEXT:    [[DOTPRE158_I:%.*]] = phi %struct.TS* [ null, [[BB1:%.*]] ], [ [[PRE2:%.*]], [[BB6:%.*]] ]
+; CHECK-NEXT:    br i1 false, label [[BB4:%.*]], label [[BB3:%.*]]
+; CHECK:       BB3:
+; CHECK-NEXT:    br label [[BB4]]
+; CHECK:       BB4:
+; CHECK-NEXT:    [[PRE1:%.*]] = phi %struct.TS* [ [[DOTPRE158_I]], [[BB3]] ], [ null, [[BB2]] ]
+; CHECK-NEXT:    br i1 false, label [[BB6]], label [[BB5:%.*]]
+; CHECK:       BB5:
+; CHECK-NEXT:    [[GEP:%.*]] = getelementptr inbounds [[STRUCT_TS:%.*]], %struct.TS* [[PRE1]], i64 0, i32 1
+; CHECK-NEXT:    [[Q3:%.*]] = load %struct.TS*, %struct.TS** [[GEP]], align 8
+; CHECK-NEXT:    br label [[BB6]]
+; CHECK:       BB6:
+; CHECK-NEXT:    [[PRE2]] = phi %struct.TS* [ null, [[BB4]] ], [ [[Q3]], [[BB5]] ]
+; CHECK-NEXT:    br label [[BB2]]
+;
+BB1:
+  br label %BB2
+
+BB2:
+  %.pre158.i = phi %"struct.TS"* [ null, %BB1 ], [ %pre2, %BB6 ]
+  br i1 false, label %BB4, label %BB3
+
+BB3:
+  br label %BB4
+
+BB4:
+  %pre1 = phi %"struct.TS"* [ %.pre158.i, %BB3 ], [ null, %BB2 ]
+  br i1 false, label %BB6, label %BB5
+
+BB5:
+  %gep = getelementptr inbounds %"struct.TS", %"struct.TS"* %pre1, i64 0, i32 1
+  %q3 = load %"struct.TS"*, %"struct.TS"** %gep, align 8
+  br label %BB6
+
+BB6:
+  %pre2 = phi %"struct.TS"* [ null, %BB4 ], [ %q3, %BB5 ]
+  br label %BB2
+}
+
+!llvm.module.flags = !{!0, !1}
+
+!0 = !{i32 7, !"openmp", i32 50}
+!1 = !{i32 7, !"openmp-device", i32 50}