[PATCH] D26892: [SCCP] Remove (maybe?) wrong code in visitBinaryOperator

[Davide Italiano via llvm-commits]

davide created this revision.
davide added a reviewer: eli.friedman.
davide added a subscriber: llvm-commits.

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.


https://reviews.llvm.org/D26892

Files:
  lib/Transforms/Scalar/SCCP.cpp
  test/Transforms/SCCP/logical-nuke.ll


Index: test/Transforms/SCCP/logical-nuke.ll
===================================================================
--- test/Transforms/SCCP/logical-nuke.ll
+++ test/Transforms/SCCP/logical-nuke.ll
@@ -1,9 +1,31 @@
-; 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
+}
Index: lib/Transforms/Scalar/SCCP.cpp
===================================================================
--- lib/Transforms/Scalar/SCCP.cpp
+++ lib/Transforms/Scalar/SCCP.cpp
@@ -928,26 +928,17 @@
       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 {
-        if (ConstantInt *CI = NonOverdefVal->getConstantInt())
-          if (CI->isAllOnesValue())     // X or -1 = -1
-            return markConstant(IV, &I, NonOverdefVal->getConstant());
+        // X or -1 = -1
+        if (NonOverdefVal->getConstantInt()->isAllOnesValue())
+          return markConstant(IV, &I, NonOverdefVal->getConstant());
       }
     }
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D26892.78647.patch
Type: text/x-patch
Size: 2240 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161120/87b9fc17/attachment.bin>