[llvm][NFC] Move content of ML subdirectory into Analysis

[mtrofin]

Summary

The initial intent was to organize ML stuff in its own directory, but it turns out that conflicts with llvm component layering policies: it is not a component, because subsequent changes want to rely on other analyses, which would create a cycle; and we don't have a reliable, cross-platform mechanism to compile files in a subdirectory, and fit in the existing LLVM build structure.

This change moves the files into Analysis, and subsequent changes will leverage conditional compilation for those that have optional dependencies.

[phosek]

llvm/cmake/modules/AddLLVM.cmake

390 ↗

(On Diff #269414)

I don't understand this part of the comment, does it mean that we always want to create OBJECT library even either STATIC or SHARED is specified?

698 ↗

(On Diff #269414)

I'm not a big fan of the name add_implementation_detail as it doesn't really match the rest of functions. How about add_llvm_internal_library or something along those lines?

[mtrofin]

Feedback

[mtrofin]

llvm/cmake/modules/AddLLVM.cmake

390 ↗

(On Diff #269414)

This documents the existing behavior. Today, this is what happens:

llvm_add_library. |. add_library parameter

STATIC | STATIC
SHARED | SHARED
STATIC SHARED. | SHARED, called ${name}, and a STATIC also, called ${name}_static
STATIC OBJECT. | STATIC called ${name}, and an OBJECT called obj.${name}. The link interface defined when calling llvm_add_library becomes

dependencies of this OBJECT target. The OBJECT target has no link interface

SHARED OBJECT | same as above, s/STATIC/SHARED
OBJECT | same as STATIC OBJECT.

On this last point, my change is technically a breaking change - but it appears nothing uses it in that particular way, i.e. OBJECT without parameters.

On this, I think we should enforce that at least one of the STATIC|OBJECT|SHARED be passed - wdyt?

698 ↗

(On Diff #269414)

sgtm!

[phosek]

llvm/cmake/modules/AddLLVM.cmake

390 ↗

(On Diff #269414)

SGTM, SHARED, STATIC and OBJECT should be three different output modes that could be combined to build multiple different outputs in a single llvm_add_library invocation, but at least one of those has to be specified.

[mtrofin]

Avoid breaking change.

[mtrofin]

llvm/cmake/modules/AddLLVM.cmake

390 ↗

(On Diff #269414)

Addressed: thinking more of it, to avoid breaking changes (and also avoid some asymmetry[1] in the API), I'd propose we add a separate flag - OBJECT_ONLY.

Checking upfront that the flags make sense - i.e. OBJECT_ONLY should come alone.

WDYT?

[1] Today's API looks at BUILD_SHARED_LIBS to figure out defaults if STATIC or SHARED aren't specified. I can see why that may be a good thing.

[mtrofin]

Is this good to go?

Thanks!

[mtrofin]

Gentle reminder - thank you!

[phosek]

LGTM

[beanz]

I have several concerns about this.

(1) This is likely going to cause issues for the Xcode builds. Xcode does not handle object libraries in a remotely sane way.
(2) How does this work with LLVM development installations where we install the static libraries for use component-by-component? CMake doesn't install object libraries.

[beanz]

It is kinda bad form to merge a change that has unaddressed feedback.

I have significant concerns about this change, and I think this change is being made to workaround incorrect implementation that landed in https://reviews.llvm.org/D80579.

We should discuss the problems with this change before any further work goes in on this.

-Chris

[mtrofin]

In D81447#2093437, @beanz wrote:

Sorry, just pushed - I can revert if needed.

I have several concerns about this.

(1) This is likely going to cause issues for the Xcode builds. Xcode does not handle object libraries in a remotely sane way.

Ack - it also fails on windows, it seems, although I'm not sure if this (http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/16497/steps/stage%201%20cmake/logs/stdio) isn't what it says it is - a stale cache. I'll try the XCode case locally (unless a bot picked it up already)

(2) How does this work with LLVM development installations where we install the static libraries for use component-by-component? CMake doesn't install object libraries.

The use case has the object library be built in its container, and then that container is the item being installed. In other words, the object library shouldn't be consumed by anything else than its container.

[mehdi_amini]

Broke our Linux build as well FYI: https://buildkite.com/mlir/mlir-core/builds/5673#854a799f-b1c2-46b4-bbca-0b1d0a5b487f

[mtrofin]

In D81447#2093555, @beanz wrote:

It is kinda bad form to merge a change that has unaddressed feedback.

Apologies again, I didn't see your feedback after the earlier LGTM, until after I pushed. There seems to be a random delay between Phabricator comments being posted, and emails being sent. I suppose I could refresh more actively the Phabricator page.

I have significant concerns about this change, and I think this change is being made to workaround incorrect implementation that landed in https://reviews.llvm.org/D80579.

We should discuss the problems with this change before any further work goes in on this.

-Chris

[mtrofin]

Alternative

[beanz]

LGTM

llvm/include/llvm/Analysis/InlineFeaturesAnalysis.h

 #ifndef LLVM_INLINEFEATURESANALYSIS_H_
 #define LLVM_INLINEFEATURESANALYSIS_H_
 
 #include "llvm/IR/PassManager.h"
 
 namespace llvm {
 class Function;
 
 class InlineFeaturesAnalysis
     : public AnalysisInfoMixin<InlineFeaturesAnalysis> {
 public:
   static AnalysisKey Key;
   struct Result {
     /// Number of basic blocks
     int64_t BasicBlockCount = 0;
 
     /// Number of blocks reached from a conditional instruction, or that are
     /// 'cases' of a SwitchInstr.
     // FIXME: We may want to replace this with a more meaningful metric, like
     // number of conditionally executed blocks:
     // 'if (a) s();' would be counted here as 2 blocks, just like
     // 'if (a) s(); else s2(); s3();' would.
     int64_t BlocksReachedFromConditionalInstruction = 0;
 
     /// Number of uses of this function, plus 1 if the function is callable
     /// outside the module.
     int64_t Uses = 0;
 
     /// Number of direct calls made from this function to other functions
     /// defined in this module.
     int64_t DirectCallsToDefinedFunctions = 0;
   };
   Result run(const Function &F, FunctionAnalysisManager &FAM);
 };
 
 } // namespace llvm
 #endif // LLVM_INLINEFEATURESANALYSIS_H_
 No newline at end of file

llvm/lib/Analysis/CMakeLists.txt

-add_subdirectory(ML)
-
 add_llvm_component_library(LLVMAnalysis
   AliasAnalysis.cpp
   AliasAnalysisEvaluator.cpp
   AliasAnalysisSummary.cpp
   AliasSetTracker.cpp
   Analysis.cpp
   AssumeBundleQueries.cpp
   AssumptionCache.cpp
add_llvm_component_library(LLVMAnalysis
   GlobalsModRef.cpp
   GuardUtils.cpp
   HeatUtils.cpp
   IVDescriptors.cpp
   IVUsers.cpp
   IndirectCallPromotionAnalysis.cpp
   InlineCost.cpp
   InlineAdvisor.cpp
+  InlineFeaturesAnalysis.cpp
   InstCount.cpp
   InstructionPrecedenceTracking.cpp
   InstructionSimplify.cpp
   Interval.cpp
   IntervalPartition.cpp
   LazyBranchProbabilityInfo.cpp
   LazyBlockFrequencyInfo.cpp
   LazyCallGraph.cpp

llvm/lib/Analysis/InlineFeaturesAnalysis.cpp

-#include "llvm/Analysis/ML/InlineFeaturesAnalysis.h"
+#include "llvm/Analysis/InlineFeaturesAnalysis.h"
 #include "llvm/IR/Instructions.h"
 
 using namespace llvm;
 
 AnalysisKey InlineFeaturesAnalysis::Key;
 
 InlineFeaturesAnalysis::Result
 InlineFeaturesAnalysis::run(const Function &F, FunctionAnalysisManager &FAM) {
for (const auto &I : BB)
       if (auto *CS = dyn_cast<CallBase>(&I)) {
         const auto *Callee = CS->getCalledFunction();
         if (Callee && !Callee->isIntrinsic() && !Callee->isDeclaration())
           ++Ret.DirectCallsToDefinedFunctions;
       }
   }
   return Ret;
 }
 No newline at end of file

llvm/lib/Analysis/LLVMBuild.txt

 ;===- ./lib/Analysis/LLVMBuild.txt -----------------------------*- Conf -*--===;
 ;
 ; Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 ; See https://llvm.org/LICENSE.txt for license information.
 ; SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 ;
 ;===------------------------------------------------------------------------===;
 ;
 ; This is an LLVMBuild description file for the components in this subdirectory.
 ;
 ; For more information on the LLVMBuild system, please see:
 ;
 ;   http://llvm.org/docs/LLVMBuild.html
 ;
 ;===------------------------------------------------------------------------===;
 
-[common]
-subdirectories = ML
-
 [component_0]
 type = Library
 name = Analysis
 parent = Libraries
 required_libraries = BinaryFormat Core Object ProfileData Support

llvm/lib/Analysis/ML/CMakeLists.txt

-add_llvm_component_library(LLVMMLPolicies
-  InlineFeaturesAnalysis.cpp
-
-  DEPENDS
-  intrinsics_gen
-  )

llvm/lib/Analysis/ML/LLVMBuild.txt

-;===- ./lib/Analysis/ML/LLVMBuild.txt --------------------------*- Conf -*--===;
-;
-; Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-; See https://llvm.org/LICENSE.txt for license information.
-; SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-;
-;===------------------------------------------------------------------------===;
-;
-; This is an LLVMBuild description file for the components in this subdirectory.
-;
-; For more information on the LLVMBuild system, please see:
-;
-;   http://llvm.org/docs/LLVMBuild.html
-;
-;===------------------------------------------------------------------------===;
-
-[component_0]
-type = Library
-name = MLPolicies
-parent = Analysis
-required_libraries = Core Support

llvm/lib/Passes/LLVMBuild.txt

 ;   http://llvm.org/docs/LLVMBuild.html
 ;
 ;===------------------------------------------------------------------------===;
 
 [component_0]
 type = Library
 name = Passes
 parent = Libraries
-required_libraries = AggressiveInstCombine Analysis MLPolicies CodeGen Core Coroutines IPO InstCombine Scalar Support Target TransformUtils Vectorize Instrumentation
+required_libraries = AggressiveInstCombine Analysis CodeGen Core Coroutines IPO InstCombine Scalar Support Target TransformUtils Vectorize Instrumentation

llvm/lib/Passes/PassBuilder.cpp

 #include "llvm/Analysis/CallGraph.h"
 #include "llvm/Analysis/DDG.h"
 #include "llvm/Analysis/DemandedBits.h"
 #include "llvm/Analysis/DependenceAnalysis.h"
 #include "llvm/Analysis/DominanceFrontier.h"
 #include "llvm/Analysis/GlobalsModRef.h"
 #include "llvm/Analysis/IVUsers.h"
 #include "llvm/Analysis/InlineAdvisor.h"
+#include "llvm/Analysis/InlineFeaturesAnalysis.h"
 #include "llvm/Analysis/LazyCallGraph.h"
 #include "llvm/Analysis/LazyValueInfo.h"
 #include "llvm/Analysis/LoopAccessAnalysis.h"
 #include "llvm/Analysis/LoopCacheAnalysis.h"
 #include "llvm/Analysis/LoopInfo.h"
 #include "llvm/Analysis/LoopNestAnalysis.h"
-#include "llvm/Analysis/ML/InlineFeaturesAnalysis.h"
 #include "llvm/Analysis/MemoryDependenceAnalysis.h"
 #include "llvm/Analysis/MemorySSA.h"
 #include "llvm/Analysis/ModuleSummaryAnalysis.h"
 #include "llvm/Analysis/OptimizationRemarkEmitter.h"
 #include "llvm/Analysis/PhiValues.h"
 #include "llvm/Analysis/PostDominators.h"
 #include "llvm/Analysis/ProfileSummaryInfo.h"
 #include "llvm/Analysis/RegionInfo.h"

llvm/unittests/Analysis/CMakeLists.txt

add_llvm_unittest(AnalysisTests
   CallGraphTest.cpp
   CaptureTrackingTest.cpp
   CFGTest.cpp
   CGSCCPassManagerTest.cpp
   DDGTest.cpp
   DivergenceAnalysisTest.cpp
   DomTreeUpdaterTest.cpp
   GlobalsModRefTest.cpp
+  InlineFeaturesAnalysisTest.cpp
   IVDescriptorsTest.cpp
   LazyCallGraphTest.cpp
   LoadsTest.cpp
   LoopInfoTest.cpp
   LoopNestTest.cpp
   MemoryBuiltinsTest.cpp
   MemorySSATest.cpp
   OrderedInstructionsTest.cpp
   PhiValuesTest.cpp
   ProfileSummaryInfoTest.cpp
   ScalarEvolutionTest.cpp
   VectorFunctionABITest.cpp
   SparsePropagation.cpp
   TargetLibraryInfoTest.cpp
   TBAATest.cpp
   UnrollAnalyzerTest.cpp
   ValueLatticeTest.cpp
   ValueTrackingTest.cpp
   VectorUtilsTest.cpp
-  )
-
-add_subdirectory(ML)
+  )
 

llvm/unittests/Analysis/InlineFeaturesAnalysisTest.cpp

 //===- InlineFeaturesAnalysisTest.cpp - inline features unit tests --------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-#include "llvm/Analysis/ML/InlineFeaturesAnalysis.h"
+#include "llvm/Analysis/InlineFeaturesAnalysis.h"
 #include "llvm/AsmParser/Parser.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Support/SourceMgr.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;

llvm/unittests/Analysis/ML/CMakeLists.txt

-set(LLVM_LINK_COMPONENTS
-  Analysis  
-  AsmParser
-  Core
-  MLPolicies
-  Support
-  TransformUtils
-  )
-
-add_llvm_unittest(MLAnalysisTests
-  InlineFeaturesAnalysisTest.cpp
-  )