rfc and [patch]: Unify -mfpu and .fpu handling, let .fpu toggle features

[Reid Kleckner]

What happens if I don't build the ARM backend? I use
-DLLVM_TARGETS_TO_BUILD=X86 in cmake, for example. I believe we have a
number of out-of-tree users of clang that do things like build Clang
without building any targets.

I think we have to suffer the duplication, or do some library splitting,
unfortunately.

This is actually very similar to a lot of problems we have. There are many
layers where we need to know something about a target (TargetTransformInfo,
anyone?) and we either work around it with late-binding-stuff (see target
registry) or duplication (see the Target*.cpp files in Clang).


On Fri, Aug 15, 2014 at 12:58 PM, Nico Weber <thakis at chromium.org> wrote:

> > What is the size impact in libclang? It links with lib/Driver, no?
>
> libclang.3.5.dylib grows by about 1.3 kB, so it's not much. (This make
> sense, since Driver only needs one symbol from a single .o file
> in libLLVMARMCodeGen.a, and that .o file doesn't need any symbols from
> other cpp files, only some enums from a .inc file it includes. So the
> linker only needs to load that single .o file.)
>
> > Clang and external tools depend on LLVM's APIs, containers and
> > algorithms, but they're all (or should be) in a highly accessible
> > area, and most certainly *not* in a source area.
>
> The new header included by clang is in
> include/llvm/Target/ARMSubtargetCommon.h , in an include directory. If
> that's not what you mean: What header are you referring to?
>
> On Fri, Aug 15, 2014 at 1:22 AM, Renato Golin <renato.golin at linaro.org>
> wrote:
>
>> On 14 August 2014 19:16, Nico Weber <thakis at chromium.org> wrote:
>> > Do folks think that this is ok as an incremental improvement than what's
>> > currently in the tree? On the bug the suggestion was made to just
>> duplicate
>> > the fpu->feature mapping in two places, which would make this patch much
>> > smaller (and which would undo the .fpu name fixes), but that seems like
>> a
>> > worse alternative to me.
>>
>> Nico,
>>
>> You're trying to solve two problems in one patch. The first problem is
>> an .fpu directive parsing issue, just has to set the bit flag, like
>> .arch, and the second is sharing the parser and bit flags with clang.
>>
>
> Well, right now the feature flags are only needed in one place. I require
> them in a second place, so I'm looking into ways to share them.
>
>
>> Sometimes, solving one problem not in the best way, and then solving
>> the other problem later is better from a VCS point of view. Also, they
>> allow people to discuss the best implementation of the second problem
>> *after* the original problem was fixed.
>>
>> I don't think anyone will agree that this is the *right* fix, but as
>> an interim state, while we discuss a better patter for the second
>> problem, it's acceptable, with a big FIXME, so that we clean that up
>> once we agreed on the second problem, that is much greater than the
>> first.
>>
>> Of course, if you're ok with the first problem waiting until we have a
>> better solution for the second problem, than let's not rush ourselves
>> and wait for a proper fix of both problems.
>>
>> But my argument so far has never been not to fix the second problem,
>> just that the fix you propose is not good enough and will create other
>> problems.
>
>
> My perspective is that my patch is better than duplicating the code,
> better than what's currently in the tree, and it doesn't make a future
> grand refactoring harder if someone wants to do that. So I much prefer my
> patch over just having a duplicate fpu name -> feature flags list in the
> arm asm parser. (The current inconsistencies between just the fpu name
> parsing between -mfpu and .fpu are a good example of this.)
>
> In my experience, "add a big FIXME and wait for a grand refactoring" leads
> to a codebase full of FIXMEs and many pending refactorings, some of which
> might never happen. I'm a bit surprised by you opposing this patch so much,
> as it fixes a problem and doesn't make any of the things you propose harder.
>
> (For comparison, here's how things would look if I just duplicate the
> mapping. The patch is way shorter, but it's very easy to update one of the
> two places when adding fpus. And with this patch, .fpu doesn't support a
> bunch of fpus that -mfpu does support – because fpu names are listed in two
> places and have fallen out of sync due to that.)
>
> Nico
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140815/32957b2e/attachment.html>