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

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

Hi @efriedma, comments are inline.



================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:110
+
+
 namespace {
----------------
efriedma wrote:
> 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?
> That implies the block dominates its predecessor, i.e. it's a loop header.
This helps me understand why I was down the path of `LoopHeader` analysis. In D63629 that's exactly what is happening: we calculate all loop headers and then immediately alter the state for trivially constant TIs causing the true loop headers to emerge to `FindFunctionBackedges()`. 

> 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.
I suspect you're right. The failing IR is unusual in several ways and I noticed this. I've never seen clang pass JT a CFG with this property.

> 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've thought about it a bit more and agree with you. It'd be better to find the latent bugs in JT if external users enable the flag in their non-standard flows. I'm fine with leaving as-is.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63913