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

[Nico Weber]

> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140815/4462975e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-feature-code-duplicated.patch
Type: application/octet-stream
Size: 1919 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140815/4462975e/attachment.obj>