Don't collect from phi nodes which are not fully constructed yet

[ZuseZ4]

This fixes two issues reported for Enzyme+Rust in EnzymeAD/rust#196,
based on help by @wsmoses

I have minimized one of the issues into a minimal IR reproducer, which crashes Enzyme when it calls llvm's scev.
EnzymeAD/Enzyme#2310 (comment)

However, I'm unsure how to make a unit test for it, since it seems like opt seems to automatically verify input while parsing.

So if I either remove a branch towards a bb with a phi node (but not the corresponding entry from the phi node), or vice versa, I get

PHINode should have one entry for each predecessor of its parent basic block!

cc @nikic since we discussed it last week

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Manuel Drehwald (ZuseZ4)

Changes

This fixes two issues reported for Enzyme+Rust in EnzymeAD/rust#196,
based on help by @wsmoses

I have minimized one of the issues into a minimal IR reproducer, which crashes Enzyme when it calls llvm's scev.
EnzymeAD/Enzyme#2310 (comment)

However, I'm unsure how to make a unit test for it, since it seems like opt seems to automatically verify input while parsing.

So if I either remove a branch towards a bb with a phi node (but not the corresponding entry from the phi node), or vice versa, I get

> PHINode should have one entry for each predecessor of its parent basic block!

cc @nikic since we discussed it last week

---

Full diff: https://github.com/llvm/llvm-project/pull/140806.diff

1 Files Affected:

• (modified) llvm/lib/Analysis/ScalarEvolution.cpp (+3-1)

diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
index 5e01c29615fab..9a35eb1312df0 100644
--- a/llvm/lib/Analysis/ScalarEvolution.cpp
+++ b/llvm/lib/Analysis/ScalarEvolution.cpp
@@ -15823,7 +15823,9 @@ void ScalarEvolution::LoopGuards::collectFromBlock(
       Depth < MaxLoopGuardCollectionDepth) {
     SmallDenseMap<const BasicBlock *, LoopGuards> IncomingGuards;
     for (auto &Phi : Pair.second->phis())
-      collectFromPHI(SE, Guards, Phi, VisitedBlocks, IncomingGuards, Depth);
+      // We don't collect from PHIs that are under construction.
+      if (Pair.second->hasNPredecessors(Phi.getNumIncomingValues()))
+        collectFromPHI(SE, Guards, Phi, VisitedBlocks, IncomingGuards, Depth);
   }
 
   // Now apply the information from the collected conditions to

[juliannagele]

Thanks for looking into this! Do I understand correctly that Enzyme ends up calling scev in a situation where the phi node doesn't match the number of predecessors, because it's still being modified? I think it should be fine to add a check for that.

You should be able to use -disable-verify to run on the broken input for writing a test

$ opt ~/tmp/phi.ll -S
PHINode should have one entry for each predecessor of its parent basic block!
  %x = phi i32 [ 0, %entry ]
phi.ll: error: input module is broken!

$ opt ~/tmp/phi.ll -S -disable-verify
define void @foo() {
...
}

[jdoerfert]

This is not an appropriate fix for the underlying issue. The passes, incl. SCEV, are not expected to work on "broken" IR. You can try to "fix" one issue that pops up after the other, but at the end of the day, this can't work at scale. If Enzyme really needs SCEV during construction, which I hope can be avoided by running SCEV queries earlier, it needs to put the IR into a valid state. For the PHI case, that might mean adding poison inputs. However, keep in mind that SCEV, and other analyses, cache results and subsequent modifications need to properly invalidate the cache or other bad things are gonna happen.

Long story short, we need to look at the Enzyme use side and make it pre-query the SCEVs, or work w/o. Queries during (generic) code-generation/modification are effectively not supported.

[ZuseZ4]

@wsmoses just fyi, I ran into this bug in another completely unrelated project. So Enzyme creating invalid IR seems like a bigger problem.

[wsmoses]

this was resolved in enzyme proper and can be closed [specifically the code was refactored to run all the analyses before generating the initial phis to be filled in, instead of the setup where they were interleaved]