[PATCH] D44177: [JumpThreading] use UnreachableBlocks to avoid unreachable regions

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

Thanks for clarifying. It would be definitely nice to see some cleanup, but I'll be fine with this happening in a follow-up patch if that makes testing easier.



================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:362
+  // waste of compute time and can potentially lead to hangs.
+  UnreachableBlocks.clear();
+  DominatorTree &DT = DDT->flush();
----------------
brzycki wrote:
> kuhar wrote:
> > I can see that `UnreachableBlocks` are being cleared every time at the beginning and at the end. Can it be a local function variable instead, or am I missing something? 
> I added `UnreachableBlocks` to `JumpThreading.h` because I used the `LoopHeaders` code as my model. Originally I was tracking `ValidBlocks` but that became memory and performance prohibitive. I can try `UnreachableBlocks` as a local variable although I'm not quite sure what the LLVM style guide is for defining a local variable differently based on `NDEBUG`.
> 
> Either way this `UnreachableBlocks.clear()` should be removed.
I think that having a different field types depending on build type is is much more problematic than having local variables of different types. This is because in the former case you may expose different ABI based on build type, while having local variables of different types is not something others can depend on. 


https://reviews.llvm.org/D44177