[PATCH] D63913: [JumpThreading] Fix threading with unusual PHI nodes.

[Eli Friedman via Phabricator via llvm-commits]

efriedma marked an inline comment as done.
efriedma added inline comments.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:110
+
+
 namespace {
----------------
brzycki wrote:
> I have concerns about adding this flag in this patch without a bit more testing and general test-cases for loop header threading. I fear this is a proverbial can of worms for any third party user of `opt` who discovers this flag and starts using it.
Thinking about it a bit more, I think it's only possible to reproduce this issue if a PHI node operand is specifically another instruction in the same basic block, since we don't rewrite any other values.  That implies the block dominates its predecessor, i.e. it's a loop header.  This means either LoopHeaders computation is disabled, or it's falling out of date due to CFG changes.

If we remove this flag, the only way to test this change is to check an IR file where a transform causes the loop headers to fall out of date, then threading happens.  Your original testcase works like this, but it's fragile: it could break in unintuitive ways due to other changes.

I'm not really concerned about adding experimental flags; it's something we do routinely, and it doesn't seem to cause any confusion.  I can rename the flag if that helps?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63913