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

[Bjorn Pettersson via llvm-commits]

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. I just wanted to correct the bug in easiest way without changing anything else.

I also read somewhere that the ReversePostOrder iteration is very expensive and should be avoided. So doing that twice seemed like a bad idea. But maybe your idea was to save the order somehow by adding another data structure?

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).

I still think my solution is a minimal impact bugfix.
If someone wants to refactor/optimise etc on this later, then I don't mind.




https://reviews.llvm.org/D26154