<p dir="ltr">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. </p>
<p dir="ltr">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. </p>
<p dir="ltr">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. </p>
<p dir="ltr">Hope that makes sense... </p>
<p dir="ltr">Cheers, <br>
Renato </p>
<div class="gmail_quote">On 15 Aug 2014 23:44, "Nico Weber" <<a href="mailto:thakis@chromium.org">thakis@chromium.org</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Aug 15, 2014 at 3:37 PM, Renato Golin <span dir="ltr"><<a href="mailto:renato.golin@linaro.org" target="_blank">renato.golin@linaro.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><p dir="ltr">Nico, </p>
<p dir="ltr">You're touching too many broken parts of both LLVM and Clang for your own good. :-) </p>
<p dir="ltr">I don't like FIXME's as much as you, but we have to pick our battles in sizes we can actually cope. </p>
<p dir="ltr">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? </p>
<p dir="ltr">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.</p></blockquote>
<div>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.<br>
</div><div><br></div><div>If that still doesn't make sense to you, I'll surrender and go ahead with the FIXME patch.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<p dir="ltr"> </p>
<p dir="ltr">Cheers, <br>
Renato </p><div><div>
<div class="gmail_quote">On 15 Aug 2014 22:39, "Nico Weber" <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Aug 15, 2014 at 2:03 PM, Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div>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.</div>
</div></blockquote><div><br></div><div>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?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">
<div>I think we have to suffer the duplication, or do some library splitting, unfortunately.<br></div><div><br></div><div>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).</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote"><div><div>On Fri, Aug 15, 2014 at 12:58 PM, Nico Weber <span dir="ltr"><<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>></span> wrote:<br>
</div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><div dir="ltr"><div><div>> What is the size impact in libclang? It links with lib/Driver, no?</div>
<div><br>
</div></div><div>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.)</div>
<div>
<div><br></div><div>> Clang and external tools depend on LLVM's APIs, containers and</div><div>> algorithms, but they're all (or should be) in a highly accessible</div><div>> area, and most certainly *not* in a source area.</div>
<div><br></div></div>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?<br><div class="gmail_extra">
<br>
</div><div class="gmail_extra"><div class="gmail_quote"><div>On Fri, Aug 15, 2014 at 1:22 AM, Renato Golin <span dir="ltr"><<a href="mailto:renato.golin@linaro.org" target="_blank">renato.golin@linaro.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>On 14 August 2014 19:16, Nico Weber <<a href="mailto:thakis@chromium.org" target="_blank">thakis@chromium.org</a>> wrote:<br>
> Do folks think that this is ok as an incremental improvement than what's<br>
> currently in the tree? On the bug the suggestion was made to just duplicate<br>
> the fpu->feature mapping in two places, which would make this patch much<br>
> smaller (and which would undo the .fpu name fixes), but that seems like a<br>
> worse alternative to me.<br>
<br>
</div>Nico,<br>
<br>
You're trying to solve two problems in one patch. The first problem is<br>
an .fpu directive parsing issue, just has to set the bit flag, like<br>
.arch, and the second is sharing the parser and bit flags with clang.<br></blockquote><div><br></div></div><div>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.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Sometimes, solving one problem not in the best way, and then solving<br>
the other problem later is better from a VCS point of view. Also, they<br>
allow people to discuss the best implementation of the second problem<br>
*after* the original problem was fixed.<br>
<br>
I don't think anyone will agree that this is the *right* fix, but as<br>
an interim state, while we discuss a better patter for the second<br>
problem, it's acceptable, with a big FIXME, so that we clean that up<br>
once we agreed on the second problem, that is much greater than the<br>
first.<br>
<br>
Of course, if you're ok with the first problem waiting until we have a<br>
better solution for the second problem, than let's not rush ourselves<br>
and wait for a proper fix of both problems.<br>
<br>
But my argument so far has never been not to fix the second problem,<br>
just that the fix you propose is not good enough and will create other<br>
problems.</blockquote><div><br></div><div></div></div></div></div><div class="gmail_extra">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.)</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">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.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">(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.)</div>
<span><font color="#888888">
<div class="gmail_extra"><br></div><div class="gmail_extra">Nico</div></font></span></div>
<br></div></div>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>
</blockquote></div><br></div></div>
</blockquote></div>
</div></div></blockquote></div><br></div></div>
</blockquote></div>