phosek added inline comments.
Comment at: cfe/trunk/CMakeLists.txt:531
+ if(BOOTSTRAP_LLVM_ENABLE_LLD)
+add_dependencies(clang-bootstrap-deps lld)
+ elseif(LLVM_BINUTILS_INCDIR)
beanz wrote:
> mehdi_amini wrote:
> > I come back to this a bit lat
beanz added inline comments.
Comment at: cfe/trunk/CMakeLists.txt:531
+ if(BOOTSTRAP_LLVM_ENABLE_LLD)
+add_dependencies(clang-bootstrap-deps lld)
+ elseif(LLVM_BINUTILS_INCDIR)
mehdi_amini wrote:
> I come back to this a bit late, sorry, but I'm
mehdi_amini added inline comments.
Comment at: cfe/trunk/CMakeLists.txt:531
+ if(BOOTSTRAP_LLVM_ENABLE_LLD)
+add_dependencies(clang-bootstrap-deps lld)
+ elseif(LLVM_BINUTILS_INCDIR)
I come back to this a bit late, sorry, but I'm not sure I unde
This revision was automatically updated to reflect the committed changes.
Closed by commit rL287179: [CMake] Support lld with LTO bootstrap (authored by
phosek).
Changed prior to commit:
https://reviews.llvm.org/D26649?vs=78096&id=78283#toc
Repository:
rL LLVM
https://reviews.llvm.org/D2664
beanz added a comment.
Yep, LGTM. Thanks!
Repository:
rL LLVM
https://reviews.llvm.org/D26649
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
mehdi_amini added a comment.
Patch still LGTM!
Thanks.
Repository:
rL LLVM
https://reviews.llvm.org/D26649
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
phosek updated this revision to Diff 78096.
phosek added a comment.
I've removed the error message and moved it to https://reviews.llvm.org/D26715.
Repository:
rL LLVM
https://reviews.llvm.org/D26649
Files:
CMakeLists.txt
Index: CMakeLists.txt
beanz added a comment.
Failing on `-DLLVM_ENABLE_LLD=ON` and `-DLLVM_ENABLE_LTO=ON` in LLVM seems fine
to me.
Repository:
rL LLVM
https://reviews.llvm.org/D26649
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-b
phosek added a comment.
In https://reviews.llvm.org/D26649#596209, @mehdi_amini wrote:
> @beanz : don't you think that it should be handled at the top level and not
> in the bootstrap logic?
> Right now we don't error when invoking cmake with `-DLLVM_ENABLE_LLD=ON`
> and `-DLLVM_ENABLE_LTO=ON
mehdi_amini added a comment.
@beanz : don't you think that it should be handled at the top level and not in
the bootstrap logic?
Right now we don't error when invoking cmake with `-DLLVM_ENABLE_LLD=ON` and
`-DLLVM_ENABLE_LTO=ON` on Darwin.
Repository:
rL LLVM
https://reviews.llvm.org/D2664
phosek updated this revision to Diff 78045.
phosek added a comment.
Added error message for Darwin, does this look good to you?
Repository:
rL LLVM
https://reviews.llvm.org/D26649
Files:
CMakeLists.txt
Index: CMakeLists.txt
beanz accepted this revision.
beanz added a comment.
Yep. Can we maybe add an error to that effect? I really don't want the build
system ignoring invalid configurations.
Otherwise LGTM!
Repository:
rL LLVM
https://reviews.llvm.org/D26649
___
cf
mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
Patch LGTM right now, let's just wait for @beanz in case he objects.
Repository:
rL LLVM
https://reviews.llvm.org/D26649
___
cfe-com
mehdi_amini added a comment.
In https://reviews.llvm.org/D26649#596024, @beanz wrote:
> On Darwin you don't want to set `DARWIN_LTO_LIBRARY` on the next stage if
> you're intending the next stage to use lld because that sets flags that
> aren't supported by lld.
Right, but that's a separate i
beanz added a comment.
On Darwin you don't want to set `DARWIN_LTO_LIBRARY` on the next stage if
you're intending the next stage to use lld because that sets flags that aren't
supported by lld.
Repository:
rL LLVM
https://reviews.llvm.org/D26649
__
phosek updated this revision to Diff 77950.
Repository:
rL LLVM
https://reviews.llvm.org/D26649
Files:
CMakeLists.txt
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -511,10 +511,10 @@
set(STAMP_DIR ${CMA
mehdi_amini added a comment.
In https://reviews.llvm.org/D26649#595429, @phosek wrote:
> AFAIK lld on Darwin doesn't support LTO (last I heard it cannot even
> self-host at the moment) so I'm not sure if it makes sense to enable it
> `if(APPLE)`.
OK, we could error instead of ignoring then.
B
phosek added a comment.
AFAIK lld on Darwin doesn't support LTO (last I heard it cannot even self-host
at the moment) so I'm not sure if it makes sense to enable it `if(APPLE)`.
Repository:
rL LLVM
https://reviews.llvm.org/D26649
___
cfe-commits
mehdi_amini added a comment.
What about the following logic?
if(BOOTSTRAP_LLVM_ENABLE_LLD)
add_dependencies(clang-bootstrap-deps lld)
endif()
if(APPLE)
# on Darwin we need to set DARWIN_LTO_LIBRARY so that -flto will work
# using the just-built compiler, and we need t
mehdi_amini requested changes to this revision.
mehdi_amini added inline comments.
This revision now requires changes to proceed.
Comment at: CMakeLists.txt:516
if(BOOTSTRAP_LLVM_ENABLE_LTO OR LLVM_ENABLE_LTO AND NOT
LLVM_BUILD_INSTRUMENTED)
-add_dependencies(clang-bootst
mehdi_amini accepted this revision.
mehdi_amini added a reviewer: mehdi_amini.
mehdi_amini added a comment.
This revision is now accepted and ready to land.
LGTM.
Repository:
rL LLVM
https://reviews.llvm.org/D26649
___
cfe-commits mailing list
cf
phosek added a comment.
In https://reviews.llvm.org/D26649#595367, @mehdi_amini wrote:
> OK, but still, LLVM_ENABLE_LLD needs to be passed to stage-2, so it needs to
> be actually BOOTSTRAP_LLVM_ENABLE_LLD.
> I looked at all the CMake `_PASSTHROUGH` and didn't find it mentioned
> anywhere. We
phosek updated this revision to Diff 77938.
phosek marked an inline comment as done.
Repository:
rL LLVM
https://reviews.llvm.org/D26649
Files:
CMakeLists.txt
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@
mehdi_amini added a comment.
In https://reviews.llvm.org/D26649#595361, @phosek wrote:
> In https://reviews.llvm.org/D26649#595356, @mehdi_amini wrote:
>
> > In https://reviews.llvm.org/D26649#595296, @phosek wrote:
> >
> > > It's sufficient, I just tested it.
> >
> >
> > How did you check it? I
phosek added a comment.
In https://reviews.llvm.org/D26649#595356, @mehdi_amini wrote:
> In https://reviews.llvm.org/D26649#595296, @phosek wrote:
>
> > It's sufficient, I just tested it.
>
>
> How did you check it? I don't understand how LLVM_ENABLE_LLD is propagated to
> stage-2?
Sufficient
mehdi_amini added a comment.
In https://reviews.llvm.org/D26649#595296, @phosek wrote:
> It's sufficient, I just tested it.
How did you check it? I don't understand how LLVM_ENABLE_LLD is propagated to
stage-2?
Comment at: CMakeLists.txt:534
+add_dependencies(LLVMgo
Eugene.Zelenko added a comment.
Yes, you are correct, I meant later.
Repository:
rL LLVM
https://reviews.llvm.org/D26649
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
mehdi_amini added a comment.
In https://reviews.llvm.org/D26649#595334, @Eugene.Zelenko wrote:
> I meant that multi-stage build is processed by higher level script. So it
> should take care about consistency of source code srt versus later stages
> build options.
We have a 2-stage build direc
Eugene.Zelenko added a comment.
I meant that multi-stage build is processed by higher level script. So it
should take care about consistency of source code srt versus later stages build
options.
Repository:
rL LLVM
https://reviews.llvm.org/D26649
_
mehdi_amini added a comment.
In https://reviews.llvm.org/D26649#595301, @Eugene.Zelenko wrote:
> I think this should be handled in higher level script
> (utils/release/test-release.sh or similar), not in CMake. CMake compiler
> tests just need to fail when LLVM_ENABLE_LLD is used without actual
Eugene.Zelenko added a comment.
I think this should be handled in higher level script
(utils/release/test-release.sh or similar), not in CMake. CMake compiler tests
just need to fail when LLVM_ENABLE_LLD is used without actually having them.
Repository:
rL LLVM
https://reviews.llvm.org/D266
mehdi_amini added a comment.
In https://reviews.llvm.org/D26649#595296, @phosek wrote:
> It's sufficient, I just tested it. I'm not actually sure if `LLVM_ENABLE_LLD`
> here is correct, `LLVM_ENABLE_LLD` forces the use of lld, but lld might not
> be available during first stage. We need `LLVM_E
phosek added a comment.
It's sufficient, I just tested it. I'm not actually sure if `LLVM_ENABLE_LLD`
here is correct, `LLVM_ENABLE_LLD` forces the use of lld, but lld might not be
available during first stage. We need `LLVM_ENABLE_LLD` to be set, but only for
the second stage (which is somethi
Eugene.Zelenko added a comment.
In https://reviews.llvm.org/D26649#595213, @mehdi_amini wrote:
> Make sense to me, but I don't think it is enough: LLVM_ENABLE_LLD should be
> passed to stage-2, and it also should be set to the absolute path of the
> stage-1 lld build.
What is search directori
mehdi_amini added inline comments.
Comment at: CMakeLists.txt:516
if(BOOTSTRAP_LLVM_ENABLE_LTO OR LLVM_ENABLE_LTO AND NOT
LLVM_BUILD_INSTRUMENTED)
add_dependencies(clang-bootstrap-deps LTO)
if(APPLE)
This dep does not make sense when using either ll
mehdi_amini added a comment.
Make sense to me, but I don't think it is enough: LLVM_ENABLE_LLD should be
passed to stage-2, and it also should be set to the absolute path of the
stage-1 lld build.
Repository:
rL LLVM
https://reviews.llvm.org/D26649
___
phosek created this revision.
phosek added a reviewer: beanz.
phosek added subscribers: cfe-commits, ruiu.
phosek set the repository for this revision to rL LLVM.
Herald added subscribers: mehdi_amini, mgorny.
lld has LTO support, if requested we should add a dependency on lld rather than
LLVMgol
37 matches
Mail list logo
