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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Analysis/LoopNestAnalysis.cpp:19
 #include "llvm/Analysis/ValueTracking.h"
+#include "llvm/Transforms/Utils/BasicBlockUtils.h"
 
----------------
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? 
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93665