[InstSimplify] Protect against more poison in SimplifyWithOpReplaced (PR47322)

[nikic]

Summary

Replace the check for poison-producing instructions in SimplifyWithOpReplaced() with the generic helper canCreatePoison() that properly handles poisonous shifts and thus avoids the problem from PR47322.

I should say that this is not really a full solution, more a minimal fix that is easy to backport to the release branch.

[aqjune]

llvm/lib/Analysis/InstructionSimplify.cpp

3799

This diff has a side effect when IIQ's UseInstrInfo is false (and I think it is okay because this change leads to less potential poison-related problems, but) when is the flag set to false?

[nikic]

llvm/lib/Analysis/InstructionSimplify.cpp

3799

Yes, this is intentional. UseInstrInfo=false should result in less folds, not more. It may be even more correct to check for || !Q.IIQ.UseInstrInfo here, i.e. make the conservative assumption that the instruction does have poison flags, if we are not allowed to inspect the actual poison flags on the instruction.

Maybe @fhahn can tell whether this is necessary.

[fhahn]

llvm/lib/Analysis/InstructionSimplify.cpp

3799

when is the flag set to false?

It was added for NewGVN, which uses the Simplify* helpers with leader values from the corresponding congruence classes, but there is no guarantee that the final of the simplified instruction is dominated by those particular instructions which has this metadata.

[aqjune]

The change looks good to me.

llvm/lib/Analysis/InstructionSimplify.cpp

3799

Thanks for the explanation! :)
For people who's seeing this patch - a relevant bug is https://llvm.org/pr37540 .

llvm/lib/Analysis/InstructionSimplify.cpp

if (auto *B = dyn_cast<BinaryOperator>(I)) {
     //   %cmp = icmp eq i32 %x, 2147483647
     //   %add = add nsw i32 %x, 1
     //   %sel = select i1 %cmp, i32 -2147483648, i32 %add
     //
     // We can't replace %sel with %add unless we strip away the flags.
     // TODO: This is an unusual limitation because better analysis results in
     //       worse simplification. InstCombine can do this fold more generally
     //       by dropping the flags. Remove this fold to save compile-time?
-    if (isa<OverflowingBinaryOperator>(B))
-      if (Q.IIQ.hasNoSignedWrap(B) || Q.IIQ.hasNoUnsignedWrap(B))
-        return nullptr;
-    if (isa<PossiblyExactOperator>(B) && Q.IIQ.isExact(B))
+    if (canCreatePoison(cast<Operator>(I)))
       return nullptr;
 
     if (MaxRecurse) {
       if (B->getOperand(0) == Op)
         return SimplifyBinOp(B->getOpcode(), RepOp, B->getOperand(1), Q,
                              MaxRecurse - 1);
       if (B->getOperand(1) == Op)
         return SimplifyBinOp(B->getOpcode(), B->getOperand(0), RepOp, Q,

llvm/test/Transforms/InstSimplify/select.ll

 ;
   %cmp9.not.1 = icmp eq i32 %x, %y
   %cmp15 = icmp slt i32 %x, %y
   %spec.select39 = select i1 %cmp9.not.1, i1 undef, i1 %cmp15
   %spec.select40 = xor i1 %cmp9.not.1, 1
   %spec.select  = and i1 %spec.select39, %spec.select40
   ret i1 %spec.select
 }
+
+define i32 @pr47322_more_poisonous_replacement(i32 %arg) {
+; CHECK-LABEL: @pr47322_more_poisonous_replacement(
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[ARG:%.*]], 0
+; CHECK-NEXT:    [[TRAILING:%.*]] = call i32 @llvm.cttz.i32(i32 [[ARG]], i1 immarg true)
+; CHECK-NEXT:    [[SHIFTED:%.*]] = lshr i32 [[ARG]], [[TRAILING]]
+; CHECK-NEXT:    [[R1_SROA_0_1:%.*]] = select i1 [[CMP]], i32 0, i32 [[SHIFTED]]
+; CHECK-NEXT:    ret i32 [[R1_SROA_0_1]]
+;
+  %cmp = icmp eq i32 %arg, 0
+  %trailing = call i32 @llvm.cttz.i32(i32 %arg, i1 immarg true)
+  %shifted = lshr i32 %arg, %trailing
+  %r1.sroa.0.1 = select i1 %cmp, i32 0, i32 %shifted
+  ret i32 %r1.sroa.0.1
+}
+declare i32 @llvm.cttz.i32(i32, i1 immarg)