Page MenuHomePhabricator

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

Authored by mtrofin on Jun 8 2020, 9:52 PM.

Details

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.

Diff Detail

Unit TestsFailed

TimeTest
160 msMLIR.Target::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/mlir-translate -mlir-to-llvmir /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/mlir/test/Target/llvmir-intrinsics.mlir | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/mlir/test/Target/llvmir-intrinsics.mlir
180 msMLIR.Target::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; c:\ws\prod\llvm-project\build\bin\mlir-translate.exe -mlir-to-llvmir C:\ws\prod\llvm-project\mlir\test\Target\llvmir-intrinsics.mlir | c:\ws\prod\llvm-project\build\bin\filecheck.exe C:\ws\prod\llvm-project\mlir\test\Target\llvmir-intrinsics.mlir

Event Timeline

mtrofin created this revision.Jun 8 2020, 9:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2020, 9:52 PM
phosek added inline comments.Jun 9 2020, 6:20 PM
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 updated this revision to Diff 269722.Jun 9 2020, 7:25 PM
mtrofin marked 3 inline comments as done.

Feedback

mtrofin marked an inline comment as done.Jun 9 2020, 7:25 PM
mtrofin added inline comments.
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 added inline comments.Jun 9 2020, 9:05 PM
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 updated this revision to Diff 269887.Jun 10 2020, 10:02 AM
mtrofin marked an inline comment as done.

Avoid breaking change.

mtrofin marked 3 inline comments as done.Jun 10 2020, 10:07 AM
mtrofin added inline comments.
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 marked an inline comment as done.Jun 11 2020, 3:42 PM

Is this good to go?

Thanks!

Gentle reminder - thank you!

phosek accepted this revision.Jun 15 2020, 10:48 AM

LGTM

This revision is now accepted and ready to land.Jun 15 2020, 10:48 AM

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.

This revision was automatically updated to reflect the committed changes.

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

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.

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 reopened this revision.Jun 15 2020, 1:35 PM
This revision is now accepted and ready to land.Jun 15 2020, 1:35 PM
mtrofin updated this revision to Diff 270848.Jun 15 2020, 1:35 PM

Alternative

mtrofin retitled this revision from [llvm] Added support for stand-alone cmake object libraries. to [llvm][NFC] Move content of ML subdirectory into Analysis.Jun 15 2020, 1:36 PM
mtrofin edited the summary of this revision. (Show Details)
beanz accepted this revision.Jun 15 2020, 2:03 PM

LGTM