[PATCH] D30322: [SCCP] Resolve indirect branch target when possible.

[Xin Tong via Phabricator via llvm-commits]

trentxintong added inline comments.


================
Comment at: lib/Transforms/Scalar/SCCP.cpp:1531-1548
+   if (auto *IBR = dyn_cast<IndirectBrInst>(TI)) {
+      if (!getValueState(IBR->getAddress()).isUnknown())
+        continue;
+
+      // If the input to SCCP is actually branch on undef, fix the undef to
+      // the first successor of the indirect branch.
+      if (isa<UndefValue>(IBR->getAddress())) {
----------------
davide wrote:
> I'm very wary of adding more code here, as it caused a fair amount of pain in the past. This, tho, seems to be in line with what was already there.
I feel getting rid of this undef resolver is the way to go for SCCP. When I read this pass, I do not feel very good about how undef is resolved, especially the ordering terminators and its input is resolved does not seem to be bullet-proof.  And it does have bugs.

However this is what we need to get indirectbr simplification working for now =).


================
Comment at: lib/Transforms/Scalar/SCCP.cpp:1895-1896
             assert(isa<UndefValue>(SI->getCondition()) && "Switch should fold");
+          } else if (auto *IBR = dyn_cast<IndirectBrInst>(I)) {
+            assert(isa<UndefValue>(IBR->getAddress()) && "IndirectBr should fold");
           } else {
----------------
davide wrote:
> Can this assertion ever fire?
TBH, I do not think IndirectBrInst, BranchInst, SwitchInst (2+ case) with Undef as input is possible at this point. As we always default-fold terminator to one target in ResolvedUndefsIn and set the input accordingly.

SwitchInst with only default-case is a slightly different, its input may not have resolved to constantint/blockaddress or overdefined state. But in such case, we make default-target always executable.

So we should only have constantint/blockaddress or constant(not int nor blockaddress)/overdefined here.

Maybe the check we really do is assert(Fold && "Must be able to fold terminator on constantint or blockaddress"); If this fails, that could mean 2 things.

1. ConstantFoldTerminator is doing something unexpected, i.e. not folding on constantint or blockaddress and not making blocks that should be dead dead.
1. This is not a terminator on constantint or blockaddress. Its on a constant or overdefined, then this block should not be dead.

In both cases, we should assert.


https://reviews.llvm.org/D30322