llvm dev - May 2019 - RFC: On removing magic numbers assuming 8-bit bytes

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

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190509/e3e635c5/attachment.html>

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.

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.

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