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

[Renato Golin]

Essentially, your patch is somewhere in between a fixme and the full thing,
only in a direction that hasn't been agreed. Going in random directions is
not in any way better than staying put, and that's why I think a lot more
people should have an opinion on it, because it IS important.

As others have noted, this patch may also break builds and will create
dependencies that will have to be removed later, and that's just not right.

Removing code is a lot harder than adding, and that's why I'm so reluctant
to adding a piece of the puzzle without knowing the full picture.

Hope that makes sense...

Cheers,
Renato
On 15 Aug 2014 23:44, "Nico Weber" <thakis at chromium.org> wrote:

> On Fri, Aug 15, 2014 at 3:37 PM, Renato Golin <renato.golin at linaro.org>
> wrote:
>
>> Nico,
>>
>> You're touching too many broken parts of both LLVM and Clang for your own
>> good. :-)
>>
>> I don't like FIXME's as much as you, but we have to pick our battles in
>> sizes we can actually cope.
>>
>> How important is the fpu fix for you? Is that breaking any builds? Can it
>> wait us planning a strawman proposal for sub arch API and implementing it
>> at least for fpus?
>>
>> We'll have to abstract away the target specific stuff, maybe via factory
>> methods, so that Clang doesn't break, but still having sub arch
>> information. But that's going to take a while.
>>
> I'll try one more time: I don't understand why my patch can't go in for
> now (assuming the "don't translate -fmpu if the arm target is disabled"
> idea mentioned above actually works), and then you can replace it with
> whatever you want when you have something to replace it with. To me, the
> codebase with that patch applied is incrementally better than where it is
> today.
>
> If that still doesn't make sense to you, I'll surrender and go ahead with
> the FIXME patch.
>
>>  Cheers,
>> Renato
>> On 15 Aug 2014 22:39, "Nico Weber" <thakis at chromium.org> wrote:
>>
>>> On Fri, Aug 15, 2014 at 2:03 PM, Reid Kleckner <rnk at google.com> wrote:
>>>
>>>> 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.
>>>>
>>>
>>> That's a good point. I haven't tried it, but it probably won't work. A
>>> (to me) reasonable fix might be to not translate -mfpu into frontend flags
>>> if the arm backend isn't enabled?
>>>
>>>
>>>> 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/76dad639/attachment.html>