[llvm] r246370 - Refactor the ARM target parsing to use a def file with macros to expand

Tue Sep 1 01:40:56 PDT 2015

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.

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.

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.

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.

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).

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.

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.

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.

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.

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.

-Chandler

On Sun, Aug 30, 2015 at 3:35 AM Renato Golin via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> On 30 August 2015 at 11:22, Chandler Carruth <chandlerc at gmail.com> wrote:
> > I don't see any activity to actually address the FIXMEs, and the macro
> > generated approach seems substantially better (less repetition, less
> prone
> > to error).
>
> We're in a stabilization period. Once the code behaves as we would
> like, I'll start the table-gen-ization, which should be pretty soon.
>
> Addressing the FIXME's is not as simple as you would imagine. Nobody
> has access to *all* ARM targets or can try out *all* variations, so
> any change has to be small and slow paced. It will take time.
>
>
> > I'm not even clear what realistic tablegen you are hoping to do here. I
> > found it hard to build an efficient parser for this using *any*
> technique.
>
> Yet another reason to not completely refactor a piece of code you had
> no history or knowledge of. If that code offends your taste (I know it
> does mine), let's discuss, not derail.
>
>
> > But arguing that we should keep the code in a bad and hard to maintain
> state
> > because of some hypothetical future design seems... not really a good
> > strategy.
>
> There's nothing hypothetical about this. We have discussed
> extensively. Search the list, ask Eric, I gave him a good run down of
> the plan and I'm really hoping I can address it before 3.8 branches.
>
> An even worse strategy is to unilaterally change a code you don't
> understand based on your view of what's good or a guess on other
> people's plans.
>
> A better approach would be: "this code is a pile of goo, here are some
> ways to fix it, shall I do it?". I'd be very glad for the offer and
> would have, then, laid out *my* plan, and I'd welcome any critics so
> that it would become *our* plan, and we'd get there faster. But right
> now, you have completely broken any hope I had to make that right, for
> whatever definition of right. You have moved back every improvement
> (for any definition of improvement) we made in the last 6 months.
>
> I don't think there's any justification for that.
>
> --renato
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150901/85b5fcbb/attachment.html>