This is an archive of the discontinued LLVM Phabricator instance.

[SCCP] Teach the pass about bitcasts
ClosedPublic

Authored by davide on Jul 7 2016, 4:53 PM.

Details

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!

Diff Detail

Event Timeline

davide updated this revision to Diff 63164.Jul 7 2016, 4:53 PM
davide retitled this revision from to [SCCP] Teach the pass about bitcasts.
davide updated this object.
davide added reviewers: eli.friedman, majnemer.
davide added a subscriber: llvm-commits.
eli.friedman edited edge metadata.Jul 7 2016, 5:42 PM

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

davide added a comment.Jul 7 2016, 5:48 PM

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 added a comment.Jul 7 2016, 6:09 PM

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?

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.

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.
This revision was automatically updated to reflect the committed changes.

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.)

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.