<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 class="h5">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 class="h5"><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>