[llvm-dev] RFC: On removing magic numbers assuming 8-bit bytes

[JF Bastien via llvm-dev]

> On May 10, 2019, at 3:57 AM, Jesper Antonsson <jesper.antonsson at ericsson.com> wrote:
> 
> On Thu, 2019-05-09 at 15:59 -0400, James Y Knight wrote:
>> From: JF Bastien via llvm-dev <llvm-dev at lists.llvm.org>
>> Date: Thu, May 9, 2019 at 1:30 PM
>> To: Jesper Antonsson
>> Cc: dag at cray.com, llvm-dev at lists.llvm.org
>> 
>>> I don’t think you have consensus to move forward at this point in
>>> time. My expectation, which I think represents LLVM’s historical
>>> approach, is that a path to full support be planned out before this
>>> effort starts. Concretely, I expect a real-world backend to be
>>> committed to LLVM as a necessary step. What I meant upthread was:
>>> yes it makes sense to do cleanups before landing a backend, but
>>> someone has to commit to upstreaming a backend before you start the
>>> cleanups. When I say a backend I don’t mean a toy, I mean a real
>>> backend.
>>> 
>> 
>> IMO, the argument of removing "magic constants", and replacing them
>> with a semantically meaningful value does have some merit, even
>> lacking any backend that uses a number other than "8".
>> 
>> I guess I'd say that my feeling is that if llvm's codebase already
>> had a "bitsPerUnit()" accessor (as GCC calls this concept), I would
>> probably not say that we should replace all the calls with literal
>> '8's across the code-base simply because no current target uses any
>> value other than 8. It's an abstraction which does make sense on its
>> own, even if it's currently always 8.
>> 
>> So, I'm somewhat agnostic here -- the change itself has little value
>> upstream, but if the state of the codebase afterwards is not degraded
>> by the change (and on the contrary, can be ever-so-marginally
>> improved), that seems like it could be basically okay.
> 
> This is a salient point, I think: If we had the abstraction, I could
> never hope to get consensus to replace it with 8. While there is no
> agreement, it seems to me that the community reaction leans toward the
> abstraction-positive.
> 
> As to the argument that a path to full support is a prerequisite, and
> that that is how it has always been done, I think every suggestion is
> worth considering on it's own merits. If this abstraction is a slight
> improvement in itself, is closing a gap towards gcc and represents a
> significant improvement for OOT backends, is there sufficient rationale
> for not allowing it?
> 
>>> Right now we have no commitment on anybody landing a backend, and
>>> we don’t really have a concrete idea of what you’re even proposing
>>> to change or how. You’re focusing on “magic numbers” like everyone
>>> agrees 8 is the root of all evil, and it’s really not. Let’s say
>>> someone promises to upstream a backend, what concretely do you need
>>> to change, and in which projects, to get there? Are you changing
>>> clang, and how? What about libc++? Linker? LLVM, and how? Is IR
>>> going to change? If not, do you keep all the i8* around, and how do
>>> you work around not having void* in IR?
>>> 
>> 
>> This is a great list of questions that it would be good to have
>> answers to, though.
> 
> By putting a patch in Phabricator and linking it from the RFC, I tried
> to convey a very concrete idea of what we propose to change and how. We
> don't propose changes outside of llvm.

A single patch isn’t a path forward, it’s one point in what I understand to be a long list. Sure your change sounds nice, but what else is needed? If all you do is upstream this then you’re leading other implementors into believing that everything else works for them. It works for you, but really nobody else, even if you’re making their lives slightly easier. You’re making it easy to break your own expectations because we have no idea what assumptions your upstream changes have. You’re going to come in to random patches, and ask for changes that are undocumented guarantees that we provide for the sake of your platform (or you’re going to proactively fix up breakage after). You say you’re going to maintain things, I want to believe you (the person), but without an actual backend upstream I find it hard to believe a company is actually committed to maintaining this.

That’s not a good way to go about.

I asked a handful of questions, I think you should answer them before moving forward. I’m sure it’s not comprehensive, therefore please document all changes you’d require. Further, I really think someone needs to commit to a real-world backend going upstream.


> The 8's are not the root of all evil, but again, magic numbers are
> generally considered bad style. Also, these 8's represent the bulk of
> our changes to handle 16-bit bytes in llvm and as they are typically
> straightforward to remove, they represent low-hanging fruit. John
> McCall mentioned that clang needs fewer changes, which is something we
> can confirm and we also experience very few merge conflicts there.
> 
> The questions are indeed good in a sense, and I could make a writeup on
> them, e.g llvm IR is general and good as it is, except that you may
> want an additon to the DataLayout string to signal AU width. However,
> as we don't suggest providing support, only this abstraction, it seems
> premature to go into these other details. Of course, when someone is
> ready to upstream a target with non-8-bit addressable units, we'll
> engage and help out with  concrete suggestions as we've been doing with
> fixedpoint numbers recently.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20190510/7412243f/attachment.html>