[PATCH] D61300: [SCCP] Fix crash when trying to constant-fold terminators multiple times.

Tue May 7 10:21:55 PDT 2019

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

In D61300#1493814 <https://reviews.llvm.org/D61300#1493814>, @fhahn wrote:

> In D61300#1493707 <https://reviews.llvm.org/D61300#1493707>, @jdoerfert wrote:
>
> > Do we need to "repair" the folded branch or could we also determine earlier that the current block is dead (it will have an unconditional branch to a dead block, right?) and avoid folding?
> >  It might make sense to fold due to the DT update though.
>
>
> We could skip calling `forceIndeterminateEdge` and `ConstantFoldTerminator`, if all successors are not executable. But I think it is better to keep the current structure, because the assertion that we managed to fold the terminator has been helpful in catching inconsistencies in the past and it is probably also quicker e.g. if you have switch statements with a large number of successors.

Now that we thought about it, I think it's fine to do it this way :)

See the comment but otherwise LGTM

================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:2087
+        auto *BB = I->getParent();
         bool Folded = ConstantFoldTerminator(I->getParent(),
                                              /*DeleteDeadConditions=*/false,
----------------
fhahn wrote:
> jdoerfert wrote:
> > now `BB`
> Do you mean omitting the `*` with `auto`? There's no reason to use `auto` here, I'll change it to `BasicBlock`.
I mean `I->getParent()` in 2087 should now be `BB`. Also, `BasicBlock` seems reasonable.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61300/new/

https://reviews.llvm.org/D61300