[SCCP] Remove useless code in visitBinaryOperator (and add tests)

[davide]

Summary

This code is very old, so I don't feel very confident touching it without a second opinion. I noticed this while I was trying to integrate undef into the solver.

The problem is that when we visit and/or, we try to derive a lattice value for the instruction even if one of the operands is overdefined.
My understanding is:
When we arrive at this point we already called getValueState() on the non-overdefined operand, so if the value is still in the 'unknown' state it means that it was found to be an 'undef' value.
I think the correct thing to do in this case is just return and wait for ResolvedUndefsIn to "plug in" the correct value, rather than calling markConstant as it's currently done.
While I was there, I added some tests for this code that were missing.

[davide]

This code was introduced by Chris in 2004 and never touched since then

commit a177c6747195f2abe5a0fc10e1a65d6c0afb85b5
Author: Chris Lattner <[e-mail]>
Date:   Sat Dec 11 23:15:19 2004 +0000

    If one side of and/or is known to be 0/-1, it doesn't matter
    if the other side is overdefined.

    This allows us to fold conditions like:  if (X < Y || Y > Z) in some cases.

cc:ing him in case he remembers the details.

[davide]

Slight correction:

When we arrive at this point we already called getValueState() on the non-overdefined operand, so if the value is still in the 'unknown' state it means that it was found to be an 'undef' value.

There are two cases:

1. the one outlined above
2. the operand hasn't been resolved yet, so we still need to wait for it.

[efriedma]

lib/Transforms/Scalar/SCCP.cpp

932

This is okay, I guess, but I don't think it fixes anything.

940

NonOverdefVal isn't guaranteed to be a ConstantInt here.

[davide]

lib/Transforms/Scalar/SCCP.cpp

932

Yes, but it was just weird to have all that logic there.

932

Renamed the revision accordingly

[efriedma]

LGTM.

[davide]

In D26892#601591, @efriedma wrote:

LGTM.

Thanks Eli.

lib/Transforms/Scalar/SCCP.cpp

932

Yes, but it was just weird to have all that logic there.

lib/Transforms/Scalar/SCCP.cpp

       NonOverdefVal = &V2State;
 
     if (NonOverdefVal) {
-      if (NonOverdefVal->isUnknown()) {
-        // Could annihilate value.
-        if (I.getOpcode() == Instruction::And)
-          markConstant(IV, &I, Constant::getNullValue(I.getType()));
-        else if (VectorType *PT = dyn_cast<VectorType>(I.getType()))
-          markConstant(IV, &I, Constant::getAllOnesValue(PT));
-        else
-          markConstant(IV, &I,
-                       Constant::getAllOnesValue(I.getType()));
+      if (NonOverdefVal->isUnknown())
         return;
-      }
 
       if (I.getOpcode() == Instruction::And) {
         // X and 0 = 0
         if (NonOverdefVal->getConstant()->isNullValue())
           return markConstant(IV, &I, NonOverdefVal->getConstant());
       } else {
+        // X or -1 = -1
         if (ConstantInt *CI = NonOverdefVal->getConstantInt())
-          if (CI->isAllOnesValue())     // X or -1 = -1
+          if (CI->isAllOnesValue())
             return markConstant(IV, &I, NonOverdefVal->getConstant());
       }
     }

test/Transforms/SCCP/logical-nuke.ll

-; RUN: opt < %s -sccp -S | grep "ret i32 0"
+; RUN: opt < %s -sccp -S | FileCheck %s
 
 ; Test that SCCP has basic knowledge of when and/or nuke overdefined values.
 
+; CHECK-LABEL: test
+; CHECK: ret i32 0
 define i32 @test(i32 %X) {
-  %Y = and i32 %X, 0    ; <i32> [#uses=1]
-  ret i32 %Y
+  %Y = and i32 %X, 0
+  ret i32 %Y
 }
 
+; CHECK-LABEL: test2
+; CHECK: ret i32 -1
+define i32 @test2(i32 %X) {
+  %Y = or i32 -1, %X
+  ret i32 %Y
+}
+
+; CHECK-LABEL: test3
+; CHECK: ret i32 0
+define i32 @test3(i32 %X) {
+  %Y = and i32 undef, %X
+  ret i32 %Y
+}
+
+; CHECK-LABEL: test4
+; CHECK: ret i32 -1
+define i32 @test4(i32 %X) {
+  %Y = or i32 %X, undef
+  ret i32 %Y
+}