[PATCH] D93665: [LoopNest] Allow empty basic blocks without loops

[Whitney Tsang via Phabricator via llvm-commits]

Whitney marked an inline comment as done.
Whitney added inline comments.


================
Comment at: llvm/lib/Analysis/LoopNestAnalysis.cpp:19
 #include "llvm/Analysis/ValueTracking.h"
+#include "llvm/Transforms/Utils/BasicBlockUtils.h"
 
----------------
Meinersbur wrote:
> Whitney wrote:
> > dblaikie wrote:
> > > While not, I think, technically impossible - this is a rather "interesting" dependency to add. (generally Analysis can't depend on Transforms, because Transforms depend on Analysis - though Transforms/Utils doesn't depend on Transforms, so we can have the diamond of Analysis depends on Transforms and Transforms and Analysis depend on Transforms/Utils)
> > > 
> > > But this is missing the CMakeLists.txt update to indicate that Analysis depends on TransformUtils.
> > > 
> > > While the dependency is technically possible, I think it might be better to revert this patch (& later ones in the series) before having a broader discussion (possibly on llvm-dev) about this new dependency due to its subtlety. Perhaps some utilities should move to Analysis or an Analysis/Utils, or an IR/Utils instead of living in Transforms/Utils, if they aren't actually doing transformations? 
> > opps, this include is no longer needed, I just forgot to remove it.
> > Thanks for pointing it out.
> I think this header is not needed anymore. It was included for `skipEmptyBlockUntil`, but this function has been moved to `Analysis/LoopNestAnalysis.h` because of the `BUILD_SHARED_LIBS` build failure which resultet from the layering violation you are referring to.
> 
> @Whitney Can you remove this #include in trunk/main?
@Meinersbur Yes, you beat me to it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93665