[PATCH] D123408: [InstCombine] Limit folding of cast into PHI

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

The problem with this patch is that it tries to carve out a special case for a specific use-case, which is something we really don't like in InstCombine...

...however, I think there is a real problem here, and it doesn't have anything to do with reductions or vectorization. What this transform is doing is convert op(phi(C, X)) into phi(op(C), op(X)), effectively just moving op from one block to another. This is usually profitable, because it reduces the paths on which op needs to be computed.

There are some exceptions to this. The first is if the X edge is critical, which is something against which the transform already guards. The second is if X is a loop backedge. In that case we either a) move an instruction from one point of a loop to another or b) we might even move an instruction from outside the loop into the loop.

The transform already has a related guard in https://github.com/llvm/llvm-project/blob/70d35fe1257e429266b83025997b400e9f79110e/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp#L1180 which prevents the a) case, which could easily result in an infinite combine loop otherwise. But it does not handle the b) case. For the test case, the transform is actually rooted at `%conv3 = zext i8 %a.0 to i32` in `for.end` (not `%conv1 = zext i8 %a.0 to i32` in `for.body`), for which the reachability check doesn't do anything.

I think the proper fix here would be to adjust that reachability check, to prevent non-profitable transforms involving loop backedges. It should probably be checking reachability from `PN->getParent()`, not `I.getParent()`.



================
Comment at: llvm/test/Transforms/InstCombine/phi-multiple-zext.ll:1
+; RUN: opt -instcombine -S %s | FileCheck %s
+
----------------
Please use update_test_checks.py...


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

https://reviews.llvm.org/D123408