<div dir="ltr"><div>Again, apologies for the delays responding. For context, and regarding the meta issues of pre- vs. post-commit, please see my response to r246367. I don’t want to split that discussion up.</div><div><br></div><div>Here, I want to focus exclusively on any and all technical concerns you have with this patch. As I said in the r246367 review thread, post-commit review is essential and frankly really refreshing to get. =] Too often, I feel like people ignore that we can and should actually review commits.</div><div><br></div><div>First, I’d like to try to explain what I’m trying to accomplish with the change. Maybe that didn’t get clearly outlined in the change description, for which I’m sorry.</div><div><br></div><div>I saw two related core issues. One is that the interface was using C-strings instead of StringRefs throughout, and the other is that when I profiled several bits of code that were calling into the ARMTargetParser, a surprisingly large amount of time was spent computing strlen() despite the fact that all the strings in the parser are constants. When analyzing this, the only really viable solution was to some how generate better constant data that can be turned directly into a StringRef, and then to push that entirely through the interface.</div><div><br></div><div>However, the only viable path to do that was to use macros to expand out the tables so that the length of the strings could be expanded as part of them. It was then pretty clear that this could also address the duplication between the enums and the tables, and the fact that the order of enums and the order of the tables must be exactly the same (and nothing previously enforced that).</div><div><br></div><div>It also didn’t seem to move us farther away from using tablegen as both the FIXMEs and the long discussion indicated was the expected final state. A very common pattern is to first move code into a preprocessor table with a .def file, and when the complexity tradeoff hits, move it further into tablegen. I was actually planning to move, consolidate, and expand on the FIXMEs to highlight that we had tabular structure that might make particular sense to generate out of TableGen at some point.</div><div><br></div><div><br></div><div>Second, I’d like to try to understand the specific concern you have with the approach I’ve used here. I’m happy to adjust it however is necessary to address your concerns, genuinely. If the macros are truly a problem, we can rip them out. I just want to understand what problem you’re seeing here. Unfortunately, I’m not sure I’m understanding your email, and so I’ve got some guesses below, but I think I’ll need you to help me better understand exactly what your concern is.</div><div><br></div><div>One possible problem I see is if this blocks subsequent moves towards a tablegen solution. While I don’t really understand all of the plans there (I know you, Eric, and others have been discussing this extensively), I can’t see any way that this blocks those plans, and if you see any I’d really like to know what they are and address them. It seems really important to not paint ourselves into a corner here.</div><div><br></div><div>Another possible problem might just be the very use of macros at all. Here, perhaps, we just have a disagreement. I think that using macro-expanded tabular data like this is actually a reasonably nice and clear pattern. It scales well in that it doesn’t require any special host tools, and can be effective at doing basic metaprogramming. Naturally, there are limits past which a proper meta-level tool like tablegen is the right approach, but it feels like this is still in the place where it is a good tool to solve the problem at hand. We use this in numerous other places in LLVM and Clang, so it didn’t seem controversial, but I’m happy to try to step through the pros and cons again.</div><div><br></div><div>The only other problem I can really see is if you’re worried that my change might change functionality in some way? I was *extremely* careful trying to make sure these were no-functionality-changed, as I understand that even with some testing in tree, we could hardly know about all possible uses of this kind of code. But if you see anything here where there is in fact a change and behavior, please call it out.</div><div><br></div><div>-Chandler</div><br><div class="gmail_quote"><div dir="ltr">On Sun, Aug 30, 2015 at 3:35 AM Renato Golin via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 30 August 2015 at 11:22, Chandler Carruth <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>> wrote:<br>
> I don't see any activity to actually address the FIXMEs, and the macro<br>
> generated approach seems substantially better (less repetition, less prone<br>
> to error).<br>
<br>
We're in a stabilization period. Once the code behaves as we would<br>
like, I'll start the table-gen-ization, which should be pretty soon.<br>
<br>
Addressing the FIXME's is not as simple as you would imagine. Nobody<br>
has access to *all* ARM targets or can try out *all* variations, so<br>
any change has to be small and slow paced. It will take time.<br>
<br>
<br>
> I'm not even clear what realistic tablegen you are hoping to do here. I<br>
> found it hard to build an efficient parser for this using *any* technique.<br>
<br>
Yet another reason to not completely refactor a piece of code you had<br>
no history or knowledge of. If that code offends your taste (I know it<br>
does mine), let's discuss, not derail.<br>
<br>
<br>
> But arguing that we should keep the code in a bad and hard to maintain state<br>
> because of some hypothetical future design seems... not really a good<br>
> strategy.<br>
<br>
There's nothing hypothetical about this. We have discussed<br>
extensively. Search the list, ask Eric, I gave him a good run down of<br>
the plan and I'm really hoping I can address it before 3.8 branches.<br>
<br>
An even worse strategy is to unilaterally change a code you don't<br>
understand based on your view of what's good or a guess on other<br>
people's plans.<br>
<br>
A better approach would be: "this code is a pile of goo, here are some<br>
ways to fix it, shall I do it?". I'd be very glad for the offer and<br>
would have, then, laid out *my* plan, and I'd welcome any critics so<br>
that it would become *our* plan, and we'd get there faster. But right<br>
now, you have completely broken any hope I had to make that right, for<br>
whatever definition of right. You have moved back every improvement<br>
(for any definition of improvement) we made in the last 6 months.<br>
<br>
I don't think there's any justification for that.<br>
<br>
--renato<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>