llvm dev - Sep 2017 - Errors linking with LLVM 5.0 - dump() missing

> On Sep 26, 2017, at 7:04 AM, David Keaton via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> On 09/25/2017 06:47 PM, David Keaton via llvm-dev wrote:
>> On 09/25/2017 06:19 PM, Matthias Braun wrote:
>>> 
>>>> On Sep 25, 2017, at 6:03 PM, David Keaton via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>>>> 
>>>> On 09/25/2017 02:53 PM, Matthias Braun via llvm-dev wrote:
>>> In the meantime `cmake
-DCMAKE_CXX_FLAGS="-DLLVM_ENABLE_DUMP"` should do the trick.
>>      Thank you.  That was the flag we needed.
>>      Unfortunately, when I tried this just now, I found that the
declaration of llvm::MachineRegisterInfo::dumpUses() in
include/llvm/CodeGen/MachineRegisterInfo.h is missing the check against
LLVM_ENABLE_DUMP.  It has only
>> #ifndef NDEBUG
>> which causes the build to fail because the definition in
lib/CodeGen/MachineRegisterInfo.cpp is guarded by this.
>> #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
> 
>     Unfortunately, this was only the beginning.  I tried updating the
header file to test LLVM_ENABLE_DUMP as well, and when I did that, the build hit
another such case, which I tried fixing, and then it hit another, and another. 
Here is what I have found so far that would need to be fixed under the current
arrangement.
> 
> LLVM_ENABLE_DUMP added to:
> 
> include/llvm/CodeGen/MachineRegisterInfo.h
> 	dumpUses()
> 
> include/llvm/CodeGen/MachineScheduler.h
> 	dumpScheduledState()
> 
> include/llvm/CodeGen/TargetSchedule.h
> 	getResourceName()
> 
> include/llvm/MC/MCSchedule.h
> 	Name
> 
> utils/TableGen/SubtargetEmitter.cpp
> 	emitted code inside EmitSchedModel()
> 
> Unknown location in AArch64
> 	This is not the end.  I just lost track of what to change at this point.
I guess that was to be expected with an option that didn't even have a cmake
flag. I would propose to just check for LLVM_ENABLE_DUMP in the sourcecode and
move the logic that debug builds have dump() methods to the cmake side of things
to simplify things. But I think that needs agreement and someone to write a
patch.

+CC some of the people who I believe made the decision for the current style.
> 
>     Perhaps it would be easier to revert the change that hid the dump()
methods.  That would have another advantage as well.  Before LLVM 5.0.0, Chapel
had the ability to link with an existing pre-built LLVM if it was installed on
the system.  With LLVM 5.0.0, we cannot do that any longer because we need to
compile with LLVM_ENABLE_DUMP defined.  It takes 10x longer to compile LLVM than
it does to compile the rest of Chapel, so it is quite a burden to require a
recompilation of LLVM to get the dump() methods.
Again: I consider client code calling dump() to be a misuse.

If you want to dump IR, can't you just switch your code from
    Value.dump();  to Value.print(dbgs(), true); dbgs() << '\n';
?

- Matthias

On 09/26/2017 11:54 AM, Matthias Braun wrote:
> If you want to dump IR, can't you just switch your code from
>      Value.dump();  to Value.print(dbgs(), true); dbgs() <<
'\n';
      Thanks.  That does seem to work for us.  Since others have 
struggled with this too, it is probably good material for the release 
notes -- possibly a good addition to make for 5.0.1.

					David

2017-09-26 11:54 GMT-07:00 Matthias Braun via llvm-dev <
llvm-dev at lists.llvm.org>:
>
> > On Sep 26, 2017, at 7:04 AM, David Keaton via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
> >
> > On 09/25/2017 06:47 PM, David Keaton via llvm-dev wrote:
> >> On 09/25/2017 06:19 PM, Matthias Braun wrote:
> >>>
> >>>> On Sep 25, 2017, at 6:03 PM, David Keaton via llvm-dev
<
> llvm-dev at lists.llvm.org> wrote:
> >>>>
> >>>> On 09/25/2017 02:53 PM, Matthias Braun via llvm-dev wrote:
> >>> In the meantime `cmake
-DCMAKE_CXX_FLAGS="-DLLVM_ENABLE_DUMP"` should
> do the trick.
> >>      Thank you.  That was the flag we needed.
> >>      Unfortunately, when I tried this just now, I found that the
> declaration of llvm::MachineRegisterInfo::dumpUses() in
> include/llvm/CodeGen/MachineRegisterInfo.h is missing the check against
> LLVM_ENABLE_DUMP.  It has only
> >> #ifndef NDEBUG
> >> which causes the build to fail because the definition in
lib/CodeGen/MachineRegisterInfo.cpp
> is guarded by this.
> >> #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
> >
> >     Unfortunately, this was only the beginning.  I tried updating the
> header file to test LLVM_ENABLE_DUMP as well, and when I did that, the
> build hit another such case, which I tried fixing, and then it hit another,
> and another.  Here is what I have found so far that would need to be fixed
> under the current arrangement.
> >
> > LLVM_ENABLE_DUMP added to:
> >
> > include/llvm/CodeGen/MachineRegisterInfo.h
> >       dumpUses()
> >
> > include/llvm/CodeGen/MachineScheduler.h
> >       dumpScheduledState()
> >
> > include/llvm/CodeGen/TargetSchedule.h
> >       getResourceName()
> >
> > include/llvm/MC/MCSchedule.h
> >       Name
> >
> > utils/TableGen/SubtargetEmitter.cpp
> >       emitted code inside EmitSchedModel()
> >
> > Unknown location in AArch64
> >       This is not the end.  I just lost track of what to change at
this
> point.
> I guess that was to be expected with an option that didn't even have a
> cmake flag. I would propose to just check for LLVM_ENABLE_DUMP in the
> sourcecode and move the logic that debug builds have dump() methods to the
> cmake side of things to simplify things. But I think that needs agreement
> and someone to write a patch.
>
> +CC some of the people who I believe made the decision for the current
> style.
>
The usual proper way to expose such options (IIRC) is to define such config
options in the llvm-config.h (or config.h I forgot which one) during the
CMake configuration, and clients are supposed to use the definition from
these header that comes with the pre-built LLVM to conditionally compile
out their calls to these.


>
> >
> >     Perhaps it would be easier to revert the change that hid the
dump()
> methods.  That would have another advantage as well.  Before LLVM 5.0.0,
> Chapel had the ability to link with an existing pre-built LLVM if it was
> installed on the system. With LLVM 5.0.0, we cannot do that any longer
> because we need to compile with LLVM_ENABLE_DUMP defined.  It takes 10x
> longer to compile LLVM than it does to compile the rest of Chapel, so it is
> quite a burden to require a recompilation of LLVM to get the dump()
methods.
Again: I consider client code calling dump() to be a
misuse.
>
I tend to agree, but there is a principled / robust way to have a client
support debug features alongside with a debug build of LLVM (the config.h
option I mentioned above).

-- 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170926/3ec03a92/attachment.html>

I've submitted a patch: https://reviews.llvm.org/D38306

Please let me know if it addressed your issues.

thanks...
don

On Tue, Sep 26, 2017 at 10:30 PM, Mehdi AMINI via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
>
> 2017-09-26 11:54 GMT-07:00 Matthias Braun via llvm-dev <
> llvm-dev at lists.llvm.org>:
>
>>
>> > On Sep 26, 2017, at 7:04 AM, David Keaton via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>> >
>> > On 09/25/2017 06:47 PM, David Keaton via llvm-dev wrote:
>> >> On 09/25/2017 06:19 PM, Matthias Braun wrote:
>> >>>
>> >>>> On Sep 25, 2017, at 6:03 PM, David Keaton via llvm-dev
<
>> llvm-dev at lists.llvm.org> wrote:
>> >>>>
>> >>>> On 09/25/2017 02:53 PM, Matthias Braun via llvm-dev
wrote:
>> >>> In the meantime `cmake
-DCMAKE_CXX_FLAGS="-DLLVM_ENABLE_DUMP"`
>> should do the trick.
>> >>      Thank you.  That was the flag we needed.
>> >>      Unfortunately, when I tried this just now, I found that
the
>> declaration of llvm::MachineRegisterInfo::dumpUses() in
>> include/llvm/CodeGen/MachineRegisterInfo.h is missing the check against
>> LLVM_ENABLE_DUMP.  It has only
>> >> #ifndef NDEBUG
>> >> which causes the build to fail because the definition in
>> lib/CodeGen/MachineRegisterInfo.cpp is guarded by this.
>> >> #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
>> >
>> >     Unfortunately, this was only the beginning.  I tried updating
the
>> header file to test LLVM_ENABLE_DUMP as well, and when I did that, the
>> build hit another such case, which I tried fixing, and then it hit
another,
>> and another.  Here is what I have found so far that would need to be
fixed
>> under the current arrangement.
>> >
>> > LLVM_ENABLE_DUMP added to:
>> >
>> > include/llvm/CodeGen/MachineRegisterInfo.h
>> >       dumpUses()
>> >
>> > include/llvm/CodeGen/MachineScheduler.h
>> >       dumpScheduledState()
>> >
>> > include/llvm/CodeGen/TargetSchedule.h
>> >       getResourceName()
>> >
>> > include/llvm/MC/MCSchedule.h
>> >       Name
>> >
>> > utils/TableGen/SubtargetEmitter.cpp
>> >       emitted code inside EmitSchedModel()
>> >
>> > Unknown location in AArch64
>> >       This is not the end.  I just lost track of what to change at
this
>> point.
>> I guess that was to be expected with an option that didn't even
have a
>> cmake flag. I would propose to just check for LLVM_ENABLE_DUMP in the
>> sourcecode and move the logic that debug builds have dump() methods to
the
>> cmake side of things to simplify things. But I think that needs
agreement
>> and someone to write a patch.
>>
>> +CC some of the people who I believe made the decision for the current
>> style.
>>
>
> The usual proper way to expose such options (IIRC) is to define such
> config options in the llvm-config.h (or config.h I forgot which one) during
> the CMake configuration, and clients are supposed to use the definition
> from these header that comes with the pre-built LLVM to conditionally
> compile out their calls to these.
>
>
>
>>
>> >
>> >     Perhaps it would be easier to revert the change that hid the
dump()
>> methods.  That would have another advantage as well.  Before LLVM
5.0.0,
>> Chapel had the ability to link with an existing pre-built LLVM if it
was
>> installed on the system. With LLVM 5.0.0, we cannot do that any longer
>> because we need to compile with LLVM_ENABLE_DUMP defined.  It takes 10x
>> longer to compile LLVM than it does to compile the rest of Chapel, so
it is
>> quite a burden to require a recompilation of LLVM to get the dump()
methods.
>
> Again: I consider client code calling dump() to be a misuse.
>>
>
> I tend to agree, but there is a principled / robust way to have a client
> support debug features alongside with a debug build of LLVM (the config.h
> option I mentioned above).
>
> --
> Mehdi
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170927/5bcd568b/attachment.html>