[PATCH] D91153: [IndVarSimplify] Fix Modified status when handling dead PHI nodes

[David Stenberg via Phabricator via llvm-commits]

dstenb added a comment.

In D91153#2393018 <https://reviews.llvm.org/D91153#2393018>, @TaWeiTu wrote:

> In D91153#2385479 <https://reviews.llvm.org/D91153#2385479>, @dstenb wrote:
>
>> I'm not very familiar with the IndVars pass, so I have not idea if this is the correct way to solve this.
>
> This seems correct to me, but I'm not familiar with IndVar either, so maybe someone else should take a look.
> However, I think it would be better if we can return `false` in this case (i.e. if all PHI nodes created in `rewriteLoopExitValues` are dead). 
> But I don't know whether there's some edge case that forces us to return `true` here.

At least with how this patch is formed, i.e. dealing with PHI nodes first, `RecursivelyDeleteDeadPHINode` will deal with some trivially dead PHI nodes that `RecursivelyDeleteTriviallyDeadInstructions` dealt with before this patch. If I just remove the `Changed |=` part for the PHI nodes, the following tests fails due to the pass return status check:

  LLVM :: Transforms/IndVarSimplify/const_phi.ll
  LLVM :: Transforms/IndVarSimplify/crash.ll
  LLVM :: Transforms/LoopSimplify/pr28272.ll

I guess that the does not really answer your question about returning `false` specifically for the `rewriteLoopExitValues` case though.

As far as I have seen in my test runs the bug that this patch fixes occurs very rarely, so perhaps it is not a huge issue to return `true` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91153