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

[Jesper Antonsson via llvm-dev]

On Fri, 2019-05-10 at 09:18 -0700, JF Bastien wrote:
> 
> 
> > 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. 

There is no long list. We only propose and plan to do what's in the
patch and very similar magic-8-removals. Honestly, no mission creep.

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

Yes, it does sound nice! And you're right, we should make a big
disclaimer at the getter, and also clearly spell out in the
documentation that there's no general support for address units/bytes
of different sizes. The risk that people who would like to use llvm for
e.g. 16-bit AUs are misled by the abstraction has to be weighed against
the fact that the very same people will be significantly helped by it,
should they choose llvm.

> It works for you, but really nobody else, even if you’re making their
> lives slightly easier.

Everyone I've talked to with some involvement in non-8-bit addressable
unit backends have been quite positive. I think it will work similarly
for us.

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

This worry about broken assumptions again seems to imply some mission
creep. To my mind, the RFC's magic number removal proposal doesn't
carry significant complexities. Yes, we will fix new magic 8-s and if
we happen to encounter patches that introduce them, we will gently ask
for them to use the abstraction. But again, no drama.

>  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 agree it's not optimal, at least, but my hope is that it's good
enough for the community to accept. Please consider the ease with which
other interested parties can help out and the low impact if a few 8-s
make their way in again.

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

With all due respect, I have to insist that this request is outside the
scope of the RFC.

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