llvm dev - Nov 2019 - RFC: On non 8-bit bytes and the target for it

> On Nov 1, 2019, at 3:41 AM, Dmitriy Borisenkov via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> A summary of the discussion so far.
> 
> It seems that there are two possible solutions on how to move forward with
non 8 bits byte:
> 
> 1. Commit changes without tests. Chris Lattner, Mikael Holmen, Jeroen
Dobbelaere, Jesper Antonsson support this idea.
> James Y Knight says that at least magic numbers should be removed "at
least where it arguably helps code clarity". This might be not exactly the
scope of the changes discussed, but it's probably worth do discuss code
clarity having concrete patches.
> GCC (according to James Y Knight) has the same practice meaning non-8 bits
byte is supported but there are no tests in upstream and we have downstream
contributors who will fix the bugs if they appear in the LLVM core.
> David Chisnall raised a question about what to count as a byte (which
defines the scope of the changes) and we suggest to use all 5 criteria he
granted:
> > - The smallest unit that can be loaded / stored at a time.
> > - The smallest unit that can be addressed with a raw pointer in a
specific address space.
> > - The largest unit whose encoding is opaque to anything above the ISA.
> > - The type used to represent `char` in C.
> > - The type that has a size that all other types are a multiple of.
> But if DSPs are less restrictive about byte, some of the criteria could be
removed.
> 
> 2. Use an iconic target. PDP10 was suggested as a candidate. This opinion
found support from Tim Northover, Joerg Sonenberger, Mehdi AMINI, Philip Reames.
It's not clear though does this opinion oppose upstreaming non-8-bits byte
without tests or just a dummy and TVM targets options.
> 
> So if there is no strong opposition to the solution 1 from the people
supporting an iconic target option, we could probably move to the patches.
I'm in camp (2). Any changes that are not tested are an invitation to
upstream developers to "simplify" the code, not knowing that those
changes are important. Anyone who commits untested changes to LLVM will
inevitably face an uphill battle against benevolent NFC refactorings that break
these changes because the expectation of how the code is supposed to behave is
not codified in a test. In the short term option (1) sounds more appealing
because they can start right away, but I'm going to predict that it will be
more expensive for the downstream maintainers of non 8-bit targets in the long
term.

-- adrian

On Fri, Nov 1, 2019 at 8:40 AM Adrian Prantl via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> > On Nov 1, 2019, at 3:41 AM, Dmitriy Borisenkov via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
> > It seems that there are two possible solutions on how to move forward
> with non 8 bits byte:
> >
> > 1. Commit changes without tests. Chris Lattner, Mikael Holmen, Jeroen
> Dobbelaere, Jesper Antonsson support this idea.
> > James Y Knight says that at least magic numbers should be removed
"at
> least where it arguably helps code clarity". This might be not exactly
the
> scope of the changes discussed, but it's probably worth do discuss code
> clarity having concrete patches.
> > GCC (according to James Y Knight) has the same practice meaning non-8
> bits byte is supported but there are no tests in upstream and we have
> downstream contributors who will fix the bugs if they appear in the LLVM
> core.
> > David Chisnall raised a question about what to count as a byte (which
> defines the scope of the changes) and we suggest to use all 5 criteria he
> granted:
> > > - The smallest unit that can be loaded / stored at a time.
> > > - The smallest unit that can be addressed with a raw pointer in a
> specific address space.
> > > - The largest unit whose encoding is opaque to anything above the
ISA.
> > > - The type used to represent `char` in C.
> > > - The type that has a size that all other types are a multiple
of.
> > But if DSPs are less restrictive about byte, some of the criteria
could
> be removed.
> >
> > 2. Use an iconic target. PDP10 was suggested as a candidate. This
> opinion found support from Tim Northover, Joerg Sonenberger, Mehdi AMINI,
> Philip Reames. It's not clear though does this opinion oppose
upstreaming
> non-8-bits byte without tests or just a dummy and TVM targets options.
> >
> > So if there is no strong opposition to the solution 1 from the people
> supporting an iconic target option, we could probably move to the patches.
>
> I'm in camp (2). Any changes that are not tested are an invitation to
> upstream developers to "simplify" the code, not knowing that
those changes
> are important. Anyone who commits untested changes to LLVM will inevitably
> face an uphill battle against benevolent NFC refactorings that break these
> changes because the expectation of how the code is supposed to behave is
> not codified in a test. In the short term option (1) sounds more appealing
> because they can start right away, but I'm going to predict that it
will be
> more expensive for the downstream maintainers of non 8-bit targets in the
> long term.
>
I've worked on multiple codebases where an option existed in order to
satisfy an extremely small userbase, with little or no testing, and as
such, I'm adamantly opposed to repeating it.

In addition to what Adrian said, where the weird option exists but is
constantly being incidentally broken, I've seen the opposite problem:
people become afraid to refactor a section of code because it might break
the weird option.  You might say "if there aren't any tests, people
should
feel free to refactor the code; their only responsibility is to make sure
the tests will still work", but honestly, I've seen the opposite:
without
tests, it's just presumed that touching certain parts of code is likely to
break things, and so after a time, an aura of untouchability starts to
surround regions of the code.  And then, the more time goes by, the more
that code becomes unfamiliar to everyone (because no one is actively
maintaining it).  In the long run, the cost of an unmaintained option may
be far more than the cost of a maintained one.

In short: please don't commit changes without tests.  Even if the test is
nothing but making sure this works:

int main(int argc, char *argv[]) {
  return argv[argc - 1][0];
}

That at least would give some freedom from the guilt of breaking something
important.

-- Jorg
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191102/bf6af4fb/attachment.html>

On Sat, Nov 2, 2019 at 12:45 AM Jorg Brown via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> On Fri, Nov 1, 2019 at 8:40 AM Adrian Prantl via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> > On Nov 1, 2019, at 3:41 AM, Dmitriy Borisenkov via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>> > It seems that there are two possible solutions on how to move
forward
>> with non 8 bits byte:
>> >
>> > 1. Commit changes without tests. Chris Lattner, Mikael Holmen,
Jeroen
>> Dobbelaere, Jesper Antonsson support this idea.
>> > James Y Knight says that at least magic numbers should be removed
"at
>> least where it arguably helps code clarity". This might be not
exactly the
>> scope of the changes discussed, but it's probably worth do discuss
code
>> clarity having concrete patches.
>> > GCC (according to James Y Knight) has the same practice meaning
non-8
>> bits byte is supported but there are no tests in upstream and we have
>> downstream contributors who will fix the bugs if they appear in the
LLVM
>> core.
>> > David Chisnall raised a question about what to count as a byte
(which
>> defines the scope of the changes) and we suggest to use all 5 criteria
he
>> granted:
>> > > - The smallest unit that can be loaded / stored at a time.
>> > > - The smallest unit that can be addressed with a raw pointer
in a
>> specific address space.
>> > > - The largest unit whose encoding is opaque to anything above
the ISA.
>> > > - The type used to represent `char` in C.
>> > > - The type that has a size that all other types are a
multiple of.
>> > But if DSPs are less restrictive about byte, some of the criteria
could
>> be removed.
>> >
>> > 2. Use an iconic target. PDP10 was suggested as a candidate. This
>> opinion found support from Tim Northover, Joerg Sonenberger, Mehdi
AMINI,
>> Philip Reames. It's not clear though does this opinion oppose
upstreaming
>> non-8-bits byte without tests or just a dummy and TVM targets options.
>> >
>> > So if there is no strong opposition to the solution 1 from the
people
>> supporting an iconic target option, we could probably move to the
patches.
>>
>> I'm in camp (2). Any changes that are not tested are an invitation
to
>> upstream developers to "simplify" the code, not knowing that
those changes
>> are important. Anyone who commits untested changes to LLVM will
inevitably
>> face an uphill battle against benevolent NFC refactorings that break
these
>> changes because the expectation of how the code is supposed to behave
is
>> not codified in a test. In the short term option (1) sounds more
appealing
>> because they can start right away, but I'm going to predict that it
will be
>> more expensive for the downstream maintainers of non 8-bit targets in
the
>> long term.
>>
>
> I've worked on multiple codebases where an option existed in order to
> satisfy an extremely small userbase, with little or no testing,
>
In those situations, were the core developers responsible for those
features/users? Yeah, if I needed to support a certain observable feature
of clang continuing to work, I'd want tests (I'm pretty serious about
testing, FWIW).

> and as such, I'm adamantly opposed to repeating it.
>
>
In addition to what Adrian said, where the weird option exists but
is
> constantly being incidentally broken, I've seen the opposite problem:
> people become afraid to refactor a section of code because it might break
> the weird option.  You might say "if there aren't any tests,
people should
> feel free to refactor the code; their only responsibility is to make sure
> the tests will still work", but honestly, I've seen the opposite:
without
> tests, it's just presumed that touching certain parts of code is likely
to
> break things, and so after a time, an aura of untouchability starts to
> surround regions of the code.  And then, the more time goes by, the more
> that code becomes unfamiliar to everyone (because no one is actively
> maintaining it).  In the long run, the cost of an unmaintained option may
> be far more than the cost of a maintained one.
>

I'm not actually opposed to this situation - LLVM as a project is pretty
happy about making big structural changes to the codebase & holding the
test coverage and downstream users accountable for ensuring quality. We
rarely avoid changes due to risk of breakage & as a community push back a
fair bit on reviewers suggesting we should - if someone can't demonstrate
the breakage in an upstream test (or has a pretty good track record of true
positives that may take some time to investigate - and thus it might be
better in the short term to revert while waiting for that evidence to be
provided) the changes tend to go in and stay in.

Yeah, I think some parts of the code may become complicated enough to
warrant separate testing - but most of the code that might move to
constants for byte width, iterate over bits to that byte width, etc, will
be tested on one value & might have bugs on other values that will be found
(or not) downstream - best effort and all. But in cases where the code to
handle novel byte widths becomes more complicated - some abstraction and
unit testing would seem quite appropriate.


> In short: please don't commit changes without tests.  Even if the test
is
> nothing but making sure this works:
>
> int main(int argc, char *argv[]) {
>   return argv[argc - 1][0];
> }
>
> That at least would give some freedom from the guilt of breaking something
> important.
>
It's hard to make sure that works in a meaningful sense in this context -
without a non-8-bit-byte target in upstream LLVM, which is the point of
contention/discussion. It's unclear if there's a suitable
target/community
to provide/maintain such a target upstream. I don't think there's a
"cheap"/stub/trivial target we could create that would provide what
you're
suggesting without bitrotting quickly and being removed (more quickly than,
I think, the sort of patches to support but not provide, non-8-bit-byte
targets).

Though it's hard to guess without seeing the sort of patches that'd be
needed.

- Dave

>
> -- Jorg
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191104/19ab17c3/attachment.html>