[llvm] True fixpoint algorithm in RS4GC (PR #75826)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-transforms

Author: Petr Maj (zduka)

<details>
<summary>Changes</summary>

Fixes a problem where the explicit marking of various instructions as conflicts did not propagate to their users. An example of this:

```
%getelementptr = getelementptr i8, <2 x ptr addrspace(1)> zeroinitializer, <2 x i64> <i64 888, i64 908>
%shufflevector = shufflevector <2 x ptr addrspace(1)> %getelementptr, <2 x ptr addrspace(1)> zeroinitializer, <4 x i32> <i32 0, i32 1, i32 2, i32 3>
%shufflevector1 = shufflevector <2 x ptr addrspace(1)> %getelementptr, <2 x ptr addrspace(1)> zeroinitializer, <4 x i32> <i32 0, i32 1, i32 2, i32 3>
%select = select i1 false, <4 x ptr addrspace(1)> %shufflevector1, <4 x ptr addrspace(1)> %shufflevector
```

Here the vector shuffles will get single base (gep) during the fixpoint and therefore the select will get a known base (gep). We later mark the shuffles as conflicts, but this does not change the base of select. This gets caught by an assert where the select's type will differ from its (wrong) base later on.

The solution in the MR is to move the explicit conflict marking into the fixpoint phase.


---
Full diff: https://github.com/llvm/llvm-project/pull/75826.diff


1 Files Affected:

- (modified) llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp (+48-46) 


``````````diff
diff --git a/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp b/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
index 40b4ea92e1ff90..76660bf88fe277 100644
--- a/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
+++ b/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
@@ -967,6 +967,44 @@ static Value *findBasePointer(Value *I, DefiningValueMapTy &Cache,
     return BDVState(BaseValue, BDVState::Base, BaseValue);
   };
 
+  // Even though we have identified a concrete base (or a conflict) for all live
+  // pointers at this point, there are cases where the base is of an
+  // incompatible type compared to the original instruction. We conservatively
+  // mark those as conflicts to ensure that corresponding BDVs will be generated
+  // in the next steps.
+
+  // this is a rather explicit check for all cases where we should mark the
+  // state as a conflict to force the latter stages of the algorithm to emit
+  // the BDVs.
+  // TODO: in many cases the instructions emited for the conflicting states
+  // will be identical to the I itself (if the I's operate on their BDVs
+  // themselves). We should exploit this, but can't do it here since it would
+  // break the invariant about the BDVs not being known to be a base.
+  // TODO: the code also does not handle constants at all - the algorithm relies
+  // on all constants having the same BDV and therefore constant-only insns
+  // will never be in conflict, but this check is ignored here. If the
+  // constant conflicts will be to BDVs themselves, they will be identical
+  // instructions and will get optimized away (as in the above TODO)
+  auto MarkConflict = [&](Instruction *I, Value *BaseValue) {
+    // II and EE mixes vector & scalar so is always a conflict
+    if (isa<InsertElementInst>(I) || isa<ExtractElementInst>(I))
+      return true;
+    // Shuffle vector is always a conflict as it creates new vector from
+    // existing ones.
+    if (isa<ShuffleVectorInst>(I))
+      return true;
+    // Any  instructions where the computed base type differs from the
+    // instruction type. An example is where an extract instruction is used by a
+    // select. Here the select's BDV is a vector (because of extract's BDV),
+    // while the select itself is a scalar type. Note that the IE and EE
+    // instruction check is not fully subsumed by the vector<->scalar check at
+    // the end, this is due to the BDV algorithm being ignorant of BDV types at
+    // this junction.
+    if (!areBothVectorOrScalar(BaseValue, I))
+      return true;
+    return false;
+  };
+
   bool Progress = true;
   while (Progress) {
 #ifndef NDEBUG
@@ -993,6 +1031,14 @@ static Value *findBasePointer(Value *I, DefiningValueMapTy &Cache,
         NewState.meet(OpState);
       });
 
+      // if the instruction has known base, but should in fact be marked as
+      // conflict because of incompatible in/out types, we mark it as such
+      // ensuring that it will propagate through the fixpoint iteration
+      auto I = cast<Instruction>(BDV);
+      auto BV = NewState.getBaseValue();
+      if (BV && MarkConflict(I, BV))
+        NewState = BDVState(I, BDVState::Conflict);
+
       BDVState OldState = Pair.second;
       if (OldState != NewState) {
         Progress = true;
@@ -1012,44 +1058,8 @@ static Value *findBasePointer(Value *I, DefiningValueMapTy &Cache,
   }
 #endif
 
-  // Even though we have identified a concrete base (or a conflict) for all live
-  // pointers at this point, there are cases where the base is of an
-  // incompatible type compared to the original instruction. We conservatively
-  // mark those as conflicts to ensure that corresponding BDVs will be generated
-  // in the next steps.
-
-  // this is a rather explicit check for all cases where we should mark the
-  // state as a conflict to force the latter stages of the algorithm to emit
-  // the BDVs.
-  // TODO: in many cases the instructions emited for the conflicting states
-  // will be identical to the I itself (if the I's operate on their BDVs
-  // themselves). We should expoit this, but can't do it here since it would
-  // break the invariant about the BDVs not being known to be a base.
-  // TODO: the code also does not handle constants at all - the algorithm relies
-  // on all constants having the same BDV and therefore constant-only insns
-  // will never be in conflict, but this check is ignored here. If the
-  // constant conflicts will be to BDVs themselves, they will be identical
-  // instructions and will get optimized away (as in the above TODO)
-  auto MarkConflict = [&](Instruction *I, Value *BaseValue) {
-    // II and EE mixes vector & scalar so is always a conflict
-    if (isa<InsertElementInst>(I) || isa<ExtractElementInst>(I))
-      return true;
-    // Shuffle vector is always a conflict as it creates new vector from
-    // existing ones.
-    if (isa<ShuffleVectorInst>(I))
-      return true;
-    // Any  instructions where the computed base type differs from the
-    // instruction type. An example is where an extract instruction is used by a
-    // select. Here the select's BDV is a vector (because of extract's BDV),
-    // while the select itself is a scalar type. Note that the IE and EE
-    // instruction check is not fully subsumed by the vector<->scalar check at
-    // the end, this is due to the BDV algorithm being ignorant of BDV types at
-    // this junction.
-    if (!areBothVectorOrScalar(BaseValue, I))
-      return true;
-    return false;
-  };
-
+  // since we do the conflict marking as part of the fixpoint iteration this
+  // loop only asserts that invariants are met
   for (auto Pair : States) {
     Instruction *I = cast<Instruction>(Pair.first);
     BDVState State = Pair.second;
@@ -1061,14 +1071,6 @@ static Value *findBasePointer(Value *I, DefiningValueMapTy &Cache,
         (!isKnownBase(I, KnownBases) || !areBothVectorOrScalar(I, BaseValue)) &&
         "why did it get added?");
     assert(!State.isUnknown() && "Optimistic algorithm didn't complete!");
-
-    // since we only mark vec-scalar insns as conflicts in the pass, our work is
-    // done if the instruction already conflicts
-    if (State.isConflict())
-      continue;
-
-    if (MarkConflict(I, BaseValue))
-      States[I] = BDVState(I, BDVState::Conflict);
   }
 
 #ifndef NDEBUG

``````````

</details>


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