[GuardWidening] Fix the crash while replacing the users of poison.

[skatkov]

Summary

When we replace poison with freeze poison it might appear
that user of poison is a constant (for example vector constant).

In this case we will get that constant will get non-constant operand.

Moreover replacing poison and GlobalValue everywhere in module seems
to be overkill. So the solution will be just make a replacement
only in instructions we visited (contributing to hoisted condition).
Moreover if user of posion is constant, this constant also should need
a freeze and it does not make sense to replace poison with frozen version,
just freeze another constant.

[mkazantsev]

Test coverage is weak. Could you please commit more tests where we freeze:

• a global used in more than one function (to make sure we don't replace with freeze in neighboring functions)
• an argument

?

Not blocking on this.

llvm/lib/Transforms/Scalar/GuardWidening.cpp

639

[&](const Use &U) { return U.getUser() != FI; });?

[nikic]

Walking users of ConstantData is not allowed -- you need to fix this in a way that doesn't perform such a walk in the first place (e.g. by bailing of out of those cases early).

[skatkov]

In D147429#4240161, @nikic wrote:

Walking users of ConstantData is not allowed -- you need to fix this in a way that doesn't perform such a walk in the first place (e.g. by bailing of out of those cases early).

will do, need to think, how to handle it.

By the way, where it is prohibited?

[nikic]

In D147429#4240214, @skatkov wrote:

In D147429#4240161, @nikic wrote:

Walking users of ConstantData is not allowed -- you need to fix this in a way that doesn't perform such a walk in the first place (e.g. by bailing of out of those cases early).

will do, need to think, how to handle it.

By the way, where it is prohibited?

See https://discourse.llvm.org/t/rfc-constantdata-should-not-have-use-lists/42606. Unfortunately, the actual assertion to enforce this never landed. It's pretty much always a bug if you do this though, if nothing else because ConstantData can have huge use lists (with millions of uses, including across modules).

[skatkov]

In D147429#4240614, @nikic wrote:

In D147429#4240214, @skatkov wrote:

In D147429#4240161, @nikic wrote:

Walking users of ConstantData is not allowed -- you need to fix this in a way that doesn't perform such a walk in the first place (e.g. by bailing of out of those cases early).

will do, need to think, how to handle it.

By the way, where it is prohibited?

See https://discourse.llvm.org/t/rfc-constantdata-should-not-have-use-lists/42606. Unfortunately, the actual assertion to enforce this never landed. It's pretty much always a bug if you do this though, if nothing else because ConstantData can have huge use lists (with millions of uses, including across modules).

ok, I see. It makes sense to re-factor this anyway. Traversing all constants and global users is compile time overhead anyway.

llvm/lib/Transforms/Scalar/GuardWidening.cpp

while (!Worklist.empty()) {
     }
     DropPoisonFlags.insert(I);
     append_range(Worklist, I->operands());
   }
   for (Instruction *I : DropPoisonFlags)
     I->dropPoisonGeneratingFlagsAndMetadata();
 
   Value *Result = Orig;
+  SmallPtrSet<Value *, 16> NeedFreezeSet(NeedFreeze.begin(), NeedFreeze.end());
   for (Value *V : NeedFreeze) {
     auto *FreezeInsertPt = getFreezeInsertPt(V, DT);
     FreezeInst *FI = new FreezeInst(V, V->getName() + ".gw.fr", FreezeInsertPt);
     ++FreezeAdded;
     if (V == Orig)
       Result = FI;
-    V->replaceUsesWithIf(FI, [&](Use & U)->bool { return U.getUser() != FI; });
+    if (isa<Instruction>(V) || isa<Argument>(V))
+      V->replaceUsesWithIf(
+          FI, [&](const Use & U)->bool { return U.getUser() != FI; });
+    else
+      // if it is a constant or global, just make a change only in instructions
+      // we visited and which are not marked as NeedFreeze itself.
+      V->replaceUsesWithIf(FI, [&](const Use & U)->bool {
+        return U.getUser() != FI && Visited.contains(U) &&
+               !NeedFreezeSet.count(U);
+      });
   }
 
   return Result;
 }
 
 bool GuardWideningImpl::widenCondCommon(Value *Cond0, Value *Cond1,
                                         Instruction *InsertPt, Value *&Result,
                                         bool InvertCondition) {

llvm/test/Transforms/GuardWidening/hang.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
 ; RUN: opt -S -passes=guard-widening < %s | FileCheck %s
 
 ; Make sure we don't hang here.
 define i64 @test() {
 ; CHECK-LABEL: define i64 @test() {
 ; CHECK-NEXT:  bb:
 ; CHECK-NEXT:    [[DOTGW_FR:%.*]] = freeze i1 poison
 ; CHECK-NEXT:    [[WIDE_CHK:%.*]] = and i1 poison, [[DOTGW_FR]]
 ; CHECK-NEXT:    call void (i1, ...) @llvm.experimental.guard(i1 [[WIDE_CHK]]) [ "deopt"() ]
 ; CHECK-NEXT:    br label [[BB2:%.*]]
 ; CHECK:       bb2:
-; CHECK-NEXT:    br i1 [[DOTGW_FR]], label [[BB3:%.*]], label [[BB2]]
+; CHECK-NEXT:    br i1 poison, label [[BB3:%.*]], label [[BB2]]
 ; CHECK:       bb3:
 ; CHECK-NEXT:    [[CALL:%.*]] = call i64 (...) @llvm.experimental.deoptimize.i64() [ "deopt"() ]
 ; CHECK-NEXT:    ret i64 [[CALL]]
 ;
 bb:
   call void (i1, ...) @llvm.experimental.guard(i1 poison) [ "deopt"() ]
   br label %bb2
 

llvm/test/Transforms/GuardWidening/posion.ll

header:
   call void (i1, ...) @llvm.experimental.guard(i1 %gc) [ "deopt"() ]
   call void (i1, ...) @llvm.experimental.guard(i1 %phi.c) [ "deopt"() ]
   %invoke.result = invoke i1 @dummy() to label %header unwind label %exception
 
 exception:
   %landing_pad = landingpad { ptr, i32 } cleanup
   ret void
 }
+
+declare void @dummy_vec(<4 x i1> %arg)
+
+define void @freeze_poison(i1 %c, i1 %g) {
+; CHECK-LABEL: @freeze_poison(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[DOTGW_FR:%.*]] = freeze i1 poison
+; CHECK-NEXT:    br i1 [[C:%.*]], label [[LEFT:%.*]], label [[RIGHT:%.*]]
+; CHECK:       left:
+; CHECK-NEXT:    call void @dummy_vec(<4 x i1> <i1 false, i1 poison, i1 poison, i1 poison>)
+; CHECK-NEXT:    ret void
+; CHECK:       right:
+; CHECK-NEXT:    [[WIDE_CHK:%.*]] = and i1 [[G:%.*]], [[DOTGW_FR]]
+; CHECK-NEXT:    call void (i1, ...) @llvm.experimental.guard(i1 [[WIDE_CHK]]) [ "deopt"() ]
+; CHECK-NEXT:    ret void
+;
+entry:
+  br i1 %c, label %left, label %right
+
+left:
+  call void @dummy_vec(<4 x i1> <i1 0, i1 poison, i1 poison, i1 poison>)
+  ret void
+
+
+right:
+  call void (i1, ...) @llvm.experimental.guard(i1 %g) [ "deopt"() ]
+  call void (i1, ...) @llvm.experimental.guard(i1 poison) [ "deopt"() ]
+  ret void
+
+}
+