[SCCP] Teach the pass about bitcasts

[davide]

Summary

My longer term goal is to remove the old interprocedural constant propagation pass code from the tree but IPCP doesn't seem to get all the cases right.
This patch is a first effort in that direction. Please take a closer look because I'm not familiar with this code and I might have gotten it completely wrong. Thanks!

[eli.friedman]

SCCPSolver::visitCastInst already handles bitcasts... what are you trying to do?

[davide]

In D22122#477611, @eli.friedman wrote:

SCCPSolver::visitCastInst already handles bitcasts... what are you trying to do?

Thanks for your review, Eli. I clearly didn't get this right :(.
To give you some more context, In the test attached to this patch the bitcast is not folded, while -constprop folds it. My question is: should -sccp/-ipcsccp be able to fold it?

[davide]

I noticed that if I replace the call in visitCastInst() from ConstExpr::getCast() to ConstantFoldCastOperand() (as I used in my previous patch)

diff --git a/lib/Transforms/Scalar/SCCP.cpp b/lib/Transforms/Scalar/SCCP.cpp
index d4c156e..346c121 100644
--- a/lib/Transforms/Scalar/SCCP.cpp
+++ b/lib/Transforms/Scalar/SCCP.cpp
@@ -760,8 +760,9 @@ void SCCPSolver::visitCastInst(CastInst &I) {
   if (OpSt.isOverdefined())          // Inherit overdefinedness of operand
     markOverdefined(&I);
   else if (OpSt.isConstant()) {
-    Constant *C =
-        ConstantExpr::getCast(I.getOpcode(), OpSt.getConstant(), I.getType());
+    Constant *C = ConstantFoldCastOperand(I.getOpcode(),
+                                        getValueState(I.getOperand(0)).getConstant(),
+                                        I.getType(), DL);
     if (isa<UndefValue>(C))
       return;
     // Propagate constant value

the value is actually folded. Is this is a reasonable change?

[eli.friedman]

I guess it's a good idea to try to fold constants as we build them... it's slightly more efficient, and it could avoid spurious "overdefined" markings in some cases. Might as well make a pass over the whole file while you're at it to find other places where we do something similar.

That said, I doubt it makes much of a difference in practice.

[davide]

In D22122#477670, @eli.friedman wrote:

I guess it's a good idea to try to fold constants as we build them... it's slightly more efficient, and it could avoid spurious "overdefined" markings in some cases. Might as well make a pass over the whole file while you're at it to find other places where we do something similar.

That said, I doubt it makes much of a difference in practice.

Thanks! I'll land this and go over the file to find other cases where we can fold as we build.
As a meta-goal, and something I'm trying to work on in the near future:

1. SCCP doesn't handle vectors. There's some code for it but it's #if 0'd . I re-enabled that code and tried to build and it passes test suite and it's able to self-host LLVM, so, I'm not entirely sure why that code is disabled and the comments in the code don't help :| Do you happen to know what's the reason?
2. SCCP doesn't process multiple value ret instructions at the moment. I plan to add support for that soon.

[eli.friedman]

SCCP doesn't handle vectors. There's some code for it but it's #if 0'd . I re-enabled that code and tried to build and it passes test suite and it's able to self-host LLVM, so, I'm not entirely sure why that code is disabled and the comments in the code don't help :| Do you happen to know what's the reason?

Just skimming the code, it looks wrong. Specifically, it looks like it's doing screwy things with undefined values. (Neither LLVM itself or the test-suite are very good tests for vector workloads.)

[davide]

In D22122#478539, @eli.friedman wrote:

SCCP doesn't handle vectors. There's some code for it but it's #if 0'd . I re-enabled that code and tried to build and it passes test suite and it's able to self-host LLVM, so, I'm not entirely sure why that code is disabled and the comments in the code don't help :| Do you happen to know what's the reason?

Just skimming the code, it looks wrong. Specifically, it looks like it's doing screwy things with undefined values. (Neither LLVM itself or the test-suite are very good tests for vector workloads.)

Thanks for the pointer. I'll try to rewrite those bits and propose a patch soon. Thanks! I'll also be sure to test on something that tests vector workloads.

lib/Transforms/Scalar/SCCP.cpp

private:
   // the value type of I to reflect these changes if appropriate.
   void visitPHINode(PHINode &I);
 
   // Terminators
   void visitReturnInst(ReturnInst &I);
   void visitTerminatorInst(TerminatorInst &TI);
 
   void visitCastInst(CastInst &I);
+  void visitBitCastInst(BitCastInst &I);
   void visitSelectInst(SelectInst &I);
   void visitBinaryOperator(Instruction &I);
   void visitCmpInst(CmpInst &I);
   void visitExtractElementInst(ExtractElementInst &I);
   void visitInsertElementInst(InsertElementInst &I);
   void visitShuffleVectorInst(ShuffleVectorInst &I);
   void visitExtractValueInst(ExtractValueInst &EVI);
   void visitInsertValueInst(InsertValueInst &IVI);
Constant *C =
         ConstantExpr::getCast(I.getOpcode(), OpSt.getConstant(), I.getType());
     if (isa<UndefValue>(C))
       return;
     // Propagate constant value
     markConstant(&I, C);
   }
 }
 
+void SCCPSolver::visitBitCastInst(BitCastInst &I) {
+  for (unsigned i = 0, e = I.getNumOperands(); i != e; ++i) {
+    LatticeVal OpSt = getValueState(I.getOperand(i));
+    if (OpSt.isOverdefined())
+      return markOverdefined(&I);
+    if (OpSt.isUndefined())
+      return;  // Operands are not resolved yet.
+  }
+  /* All the operands are known to be constant */
+  Constant *C = ConstantFoldCastOperand(I.getOpcode(),
+                                        getValueState(I.getOperand(0)).getConstant(),
+                                        I.getType(), DL);
+  if (isa<UndefValue>(C))
+    return;
+  markConstant(&I, C);
+}
 
 void SCCPSolver::visitExtractValueInst(ExtractValueInst &EVI) {
   // If this returns a struct, mark all elements over defined, we don't track
   // structs in structs.
   if (EVI.getType()->isStructTy())
     return markAnythingOverdefined(&EVI);
 
   // If this is extracting from more than one level of struct, we don't know.

test/Transforms/SCCP/bitcast.ll

+; RUN: opt < %s -ipsccp -S | FileCheck %s
+
+define i128 @vector_to_int_cast() {
+  %A = bitcast <4 x i32> <i32 1073741824, i32 1073741824, i32 1073741824, i32 1073741824> to i128
+  ret i128 %A
+}
+
+; CHECK: define i128 @vector_to_int_cast(
+; CHECK-NEXT:  ret i128 85070591750041656499021422275829170176