[PATCH] D26154: [Reassociate] Skip analysis of dead code to avoid infinite loop.

[Daniel Berlin via llvm-commits]

On Mon, Oct 31, 2016 at 11:14 AM, Bjorn Pettersson <
bjorn.a.pettersson at ericsson.com> wrote:

> bjope added inline comments.
>
>
> ================
> Comment at: lib/Transforms/Scalar/Reassociate.cpp:2182-2185
> +    // Skip dead basic blocks.
> +    if (RankMap.count(&*BI) == 0)
> +      continue;
> +
> ----------------
> dberlin wrote:
> > davide wrote:
> > > You may want to expand this comment a bit.
> > There is also a much simpler and more consistent fix:
> >
> > Iterate in the same order rankmap was built.
> >
> > You can share the ReversePostOrder ordering, and use it here, and then
> both are doing the same thing, so there can't be any mismatches.
> >
> Ok. I have no idea if the iteration order in here in ReassociatePass::run
> is important or not, so I did not dare to do such a change.


Better to see, and then do this if we can't change that easily.

> I just wanted to correct the bug in easiest way without changing anything
> else.
>
> Easiest may not be the best though ;)


> I also read somewhere that the ReversePostOrder iteration is very
> expensive and should be avoided.


This is a pretty old comment.

So doing that twice seemed like a bad idea. But maybe your idea was to save
> the order somehow by adding another data structure?
>

Yes, just save the mapping, we do this elsewhere.



>
> If it does not matter in which order the basic blocks are analysed, then I
> guess it is possible to iterate over the keys in the RankMap map (however,
> I'm not familiar about the DenseMap iterators so I do not know about if
> that is cheap, if order is predictable when debugging different builds,
> etc).
>

It's not deterministic, so you can't do it.

>
> I still think my solution is a minimal impact bugfix.
>

I would say it's the easiest. It would be appreciated if you were willing
to try to come up with something slightly better.


> If someone wants to refactor/optimise etc on this later, then I don't mind.
>
>
>
>
> https://reviews.llvm.org/D26154
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161031/ef32fca9/attachment.html>