[llvm] r246367 - [Triple] Stop abusing a class to have only static methods and just use

[Renato Golin via llvm-commits]

On 1 September 2015 at 09:38, Chandler Carruth <chandlerc at gmail.com> wrote:
> Sorry for the delay responding, I've been ill for the last day and am quite
> behind on a number of things.

I'm sorry to hear that, and I also apologise for my reaction, it was
out of place.


> Rather than responding in-line, I'm going to try to summarize the issues
> because I think there are at least three separate issues here and I want to
> address them separately. I'm not sure I can organize my thoughts effectively
> by trying to respond in-line. Hopefully this summary will be a good basis
> for continuing to discuss this, but if I've missed anything, please point it
> out, it wasn't intended.

On the same note, I'd like to first sum up my views, so that we don't
end up with two long threads, agreeing with each other, but still
arguing.

I think that keeping your patches in, as they are, is the best way
forward. Because...

1. It has generated enough fuzz already...
2. Not everything is wrong with it, and it's possible that nothing is
wrong, it's hard to tell for now (see later)...
3. The problem was never the contents of the patch, but how quick and
subtle it came in...


> 1) I was landing patches to code which I *am* actually very familiar with.
> I've worked extensively on the Triple code, Clang's driver code (which is
> where much of this code came from originally), and generally on LLVM's
> support and ADT libraries. I have worked with post-commit review on very
> many parts of them.

I was talking about the specific parts of ARM triples, which are
probably the worse nightmares of all triples. Not the parsing code,
not the triple logic, but the implicit ARM meaning of things, which no
one really knows entirely.


> 3) My patches were carefully preserving the existing functionality of the
> code. I made extremely certain that this would not regress functionality
> that people could be depending on from the existing code.

That's the other point. It's really hard to know that, because so many
people depend on the very specific meanings of each of ARM's
idiosyncrasies, and most of them are not tested anywhere in
Clang/LLVM.

There are multiple patches in Android and the Kernel which are showing
us many problems, and we're triaging each, one by one, to make sure
that any change won't break someone else's code that is not visible to
us yet.

That's why we're doing one patch at a time, and letting the code rest
for a while, so that we have time to pick those up. And we have picked
a lot of things that weren't been tested, and hopefully they should be
tested now.


> This is why I felt that it was appropriate to commit without pre-commit
> review. I stand by this decision. I'm happy for some of the other long-term
> maintainers of this part of LLVM and Clang (because the code really is
> clearly intertwined with the Clang driver) to point out why they disagree,
> but I would want some specific concerns raised in order for it to be clear
> how to make such decisions in the future.

The relation to Clang is intentional. That code was removed from Clang
and other parts of LLVM into one big mess. In essence, we moved all
problems into a single place, which as you spotted well, was still a
mess, albeit a localised one.


> But of course, post-commit review is always an important thing! =D That
> brings us to the second issue: should this code use a namespace or a class
> with static methods.

Right, the technical details...

Again, I'm going to be brief to avoid long threads:

First, there's nothing wrong with macros or namespace functions and
static classes are bad, I agree.

But, in this case, because this will be inserted into TargetTuple, and
I need some of those methods to be inherited depending on the triple's
arch, I need some flexibility.

I totally agree (and have said so in my original commit) that the
static class is a bad design, but the practical aspect of it
(compilation speed, generated code) would be very similar, and that
would allow me to move it into a proper class structure later. Only
that never happened.

I also agree that most of the arch-specific crud can remain as
def/macro/namespace functions and only the really shared stuff to be
in a light-weight class. I didn't foresee that this would get *so*
big, so fast, and was barely coping with patches by other people with
different points of view, which made the design a non-unit concept,
but an evolved mess.


So, here are some explanations of what happened...

There was *a lot* of parsing and special logic in Clang wrt ARM
triples, CPU names, arch names, etc. The initial move was simple and
straightforward. But by slowly moving all that logic into a single
place, we made a normal looking pseudo-class become a beast with 7
heads. The beast was there all along, but spread all over Clang and
the ARM back-end, we just made is visible to put everything in one
place.

One of my design goals was to make it into a table-gen back-end, so
that we could re-use the knowledge in the td files to generate a
complete set for the parser. More importantly, we'd unify the
knowledge, that today is still disconnected. Front-end options only
align to back-end decisions when we're either conscious or lucky.
There is no guarantee that they will. But I was focusing on having one
table-gen back-end for all of it, so having multiple .def files for
multiple uses would defeat the purpose of the change, and would make
us go back having the knowledge spread all over the compiler again.

I know that's not what you've done here, but in the past, when I have
accepted one satellite def file in the ARM back-end, others popped up
like a disease. Removing those files was as important as cleaning up
Clang for the TargetParser's core design goal. So seeing more def
files popping up kind of gave me a creepy feeling. It passed now. :)

Finally, as I said before, the delay is intentional. But recent delays
were made worse by the fact that the TargetTuple class' review is
stalled and we're in the holiday season, so there's at least a couple
of people offline throughout July/August, including my own holidays
that started last Monday and will end tomorrow, of which I could only
enjoy 2 days (including weekends).

One of the assumptions you did get wrong was that the code was stale. It wasn't.

Another wrong assumption is that this was either bad design or zero
maintenance, when in fact, it was slow improvements over several
months, from a much worse design.

My take away of all this is that now, it doesn't matter what the
problems will be, they can't be worse than to argue about the specific
implementation details over and over again. It's simply not worth it.

Let's continue where we stopped, and hopefully this part of the code
won't offend many people (speed, quality, design) so that we can
continue threading the minefield of ARM triples on our own pace.



> The third issue I see is best discussed on the other review thread, and that
> is the issue of using a .def file and macros in order to effectively reduce
> the need for dynamic 'strlen' calls and the usage of C-strings in the
> interfaces here. I'll keep discussion of those issues on that thread, but
> will try to focus in this thread solely on the first two issues I've
> outlined.

I said everything I wanted here, and I agree with most of your points
in the other thread. There are, however, two hidden variables there:

1. As you noticed, in-tree testing and staring at code is not enough
to catch all silly idiosyncrasies of ARM nomenclature abuse. Any big
change may hide subtle changes that even the best of us can't see. I'm
not saying this *did* happen, nor I'm saying that if I stare at the
code long enough I'll find some. We just have to let is rest and wait
for people to complain.

2. The current changes that were being reviewed (for months) will have
to pull the hand break, take a U turn, and re-write everything. By
heavily refactoring that piece of code, you made other people's work
invalid. That's not such a strong point as it may seem I'm implying, I
know, since that happens every day in LLVM. But because this is a
sensitive area, it can be a lot more destructive than usual.

If I may come up with an analogy, it's like completely replacing the
AES implementation in a library. The old one had bugs, which were
abused (not always intentionally) and that's the behaviour that most
of the users were expecting. By replacing the implementation, you're
not making the same mistakes and you're making new mistakes, both of
which will lead to hard-to-find bugs, which could only show up in
years from now and could waste days, weeks or months of many engineers
from now on.

I'm not saying this *is* what happened. I cannot possibly tell, even
if I were to search for it with a microscope. I'm just saying that,
most of the time, your decision would have made total sense. This
time, there were hidden variables that, no matter how much care you
took, could still break in odd ways and waste a lot of time because
this area is more sensitive than others.

But right now, the best course of cation is to handle them as they
come, if they come. Anything else would be pure speculation and a
waste of our time.

Thanks for your replies, I appreciate the technical discussion.

cheers,
--renato