[PATCH] Fix LTO handling of module-level assembly (Bug 14152)

[Tom Roeder]

Thanks! BTW, I just noticed that the bug number in the subject has two
numbers transposed: it should be 14512 (as in the link in my first
email), but I wrote 14152. What is the process for closing that bug?


Tom

On Thu, Sep 19, 2013 at 3:19 PM, Peter Collingbourne <peter at pcc.me.uk> wrote:
> LGTM, committed with some minor changes in r191042.
>
> Peter
>
> On Wed, Sep 18, 2013 at 05:17:58PM -0700, Tom Roeder wrote:
>> Thanks again. Here is a new version of the patch that removes
>> LTO_IS_ENABLED and adds XFAIL for win32. It has been tested with clean
>> builds in both the cmake and the Makefile versions. I've fixed the
>> 80-col problems, have added a -o flag to llvm-lto, and have moved it
>> into tools from utils. This patch also changes several of the
>> CMake/Makefiles around llvm-lto to hook it into the build.
>>
>> Tom
>>
>> On Tue, Sep 17, 2013 at 3:10 PM, Peter Collingbourne <peter at pcc.me.uk> wrote:
>> > Tom,
>> >
>> > There are a still a few more issues with this patch:
>> >
>> >  - Since the test no longer has a dependency on the gold plugin and libLTO is
>> >    built unconditionally (except on Windows), you can drop the LTO_IS_ENABLED
>> >    variable (which isn't used anywhere else, and won't work in the CMake build)
>> >    and XFAIL the test on win32.
>> >  - Have you tried running a clean build with your patch?  I think the Makefile
>> >    build has some ordering issues which require anything which depends on
>> >    something in tools to also live in tools. (It would probably be better for
>> >    llvm-lto to live in tools anyway along with the other compiler-hacker tools.)
>> >  - Please make sure llvm-lto builds with CMake (add a CMakeLists.txt file and
>> >    update {tools,utils}/CMakeLists.txt).
>> >  - llvm-lto.cpp has some 80-col violations.
>> >  - It would be better to allow a caller of llvm-lto to specify an output
>> >    file with -o, rather than just letting it write to some random temporary
>> >    file and cluttering up the user's temporary directory.  You can do that by
>> >    using lto_codegen_compile and writing the file yourself (using raw_ostream).
>> >
>> > Thanks,
>> > Peter
>> >
>> > On Tue, Sep 17, 2013 at 01:57:47PM -0700, Tom Roeder wrote:
>> >> Thanks for the feedback. Here's an updated patch that adds a simple
>> >> utility (llvm-lto) along these lines in utils/ and changes the test to
>> >> use this utility instead of ld.
>> >>
>> >> On Mon, Sep 16, 2013 at 6:33 PM, Peter Collingbourne <peter at pcc.me.uk> wrote:
>> >> > Hi Tom,
>> >> >
>> >> > I don't think that is the correct way to write the test, for a couple
>> >> > of reasons:
>> >> >
>> >> > - The test will fail if 'ld' is not gold on the user's system.
>> >> > - The test will not run on non-Linux systems.
>> >> >
>> >> > What I would suggest doing is writing a small command line test harness
>> >> > for the libLTO library, using the API defined in include/llvm-c/lto.h.
>> >> > The test harness could have a command line interface along the
>> >> > lines of:
>> >> >
>> >> > $ lto-test -o foo.o foo1.bc foo2.bc  # Link foo1.bc and foo2.bc to produce foo.o
>> >> >
>> >> > You could then use lto-test from your lit test, instead of ld.
>> >> >
>> >> > Peter
>> >> >
>> >> > On Mon, Sep 16, 2013 at 01:17:09PM -0700, Tom Roeder wrote:
>> >> >> Here's a patch that adds the test in LLVM IR to test/tools/lto with a
>> >> >> lit.local.cfg that checks if LTO is present. Also, the check for LTO
>> >> >> seems wrong to me in test/Makefile: it looks like it's checking to see
>> >> >> if -flto is in the command-line flags for the current build (rather
>> >> >> than -flto being possible with the compiler that was built). This
>> >> >> patch changes that to look for LLVMgold.so in the lib directory.  No
>> >> >> other tests seem to be using the flag lto_is_enabled at the moment.
>> >> >> Please let me know what you think.
>> >> >>
>> >> >> Tom
>> >> >>
>> >> >> On Fri, Sep 13, 2013 at 5:30 PM, Tom Roeder <tmroeder at google.com> wrote:
>> >> >> > OK, thanks. I'll figure out another place to put it.
>> >> >> >
>> >> >> > On Sep 13, 2013 5:05 PM, "Bill Wendling" <wendling at apple.com> wrote:
>> >> >> >>
>> >> >> >> On Sep 13, 2013, at 4:54 PM, Tom Roeder <tmroeder at google.com> wrote:
>> >> >> >>
>> >> >> >> > Here is a pair of patches that fix and test
>> >> >> >> > http://llvm.org/bugs/show_bug.cgi?id=14512. The fix applies to LLVM,
>> >> >> >> > and I have currently put the test in the clang test suite. The test is
>> >> >> >> > a minor tweak to the test proposed in the bug. Please take a look.
>> >> >> >> >
>> >> >> >> > The problem in this case is that the RecordStreamer implementation of
>> >> >> >> > MCStreamer in the LTO module doesn't implement EmitCFIEndProcImpl, and
>> >> >> >> > the default implementation in MCStreamer doesn't finish the frame.
>> >> >> >> >
>> >> >> >> > This patch touches the LTO code in tools/lto, but the test uses clang.
>> >> >> >> > Is this the right way to do it? Is there a better location for LTO
>> >> >> >> > tests?
>> >> >> >> >
>> >> >> >> Hi Tom,
>> >> >> >>
>> >> >> >> I haven't looked at the patch yet. However, the rule-of-thumb for where
>> >> >> >> tests should go is that they should be in located where the code that tests
>> >> >> >> them is. Clang doesn't have anything to do with LTO, so putting it in the
>> >> >> >> clang test suite isn't really good. You'll want to generate a .ll file and
>> >> >> >> place it in the LLVM part. That said, it's hard to know where such a test
>> >> >> >> would go, since we don't really have many for LTO...But just pick a good
>> >> >> >> place that's kind of related. :-)
>> >> >> >>
>> >> >> >> -bw
>> >> >> >>
>> >> >> >>
>> >> >> >
>> >> >
>> >> >
>> >> >> _______________________________________________
>> >> >> llvm-commits mailing list
>> >> >> llvm-commits at cs.uiuc.edu
>> >> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >> >
>> >> >
>> >> > --
>> >> > Peter
>> >
>> >
>> >
>> > --
>> > Peter
>
>
>
> --
> Peter