[PATCH v2 0/14] [x86] Fix 16-bit addressing modes (PR18220) and implement .code16 (PR8684)

[Eric Christopher]

>
>
> > 1) Comments should be full sentences including punctuation when
> > possible.
>
> That would be my normal style. Did I add some that weren't?
>
>
A couple. Not many though.

// No displacement

for example.


> > 2) When referencing outside sources it's often best to explain in more
> > detail what's going on in the particular section as if the external
> > reference had evaporated.
>
> Again I'm not entirely sure where you see something to provoke this
> comment. Perhaps in the first patch where I add:
>
>   // 16-bit addressing forms of the ModR/M byte have a different encoding
> for
>   // the R/M field and are far more limited in which registers can be used.
>   if (Is16BitMemOperand(MI, Op)) {
>     if (BaseReg) {
>       // See Table 2-1 "16-Bit Addressing Forms with the ModR/M byte"
>       static const int R16Table[] = { 0, 0, 0, 7, 0, 6, 4, 5 };
>   ...
>
> ... and you're looking at the second comment in there? I was mostly just
> following the precedent where we later refer to table 2-7 in much the
> same way. There's a limit to how much of the table we really want to
> reproduce locally, surely? The interesting bit is right there in the
> R16Table[] array...
>
>
It is, but it required quite a bit of staring at the table to figure out
what the array was doing. Perhaps you could elaborate on the comment and
say what's actually stored in the table, etc?


> Or was there something else?
>

That patch was the primary motivator of the comment, yes. Table 2-1 is...
not fun to look at.


>
> > 3) I appreciate the splitting up of the patch into multiple and easily
> > reviewable chunks.
> >
> >
> > 4) Testcases should be part of the diff that adds the functionality
> > (in particular the ones adding instructions for 16-bit mode).
>
> Is that mandatory? As you can see, I mostly just copied the x86-32.s
> file and tweaked it (and compared the output with gas to make sure I
> wasn't putting in any "test for the wrong output" tests like the one I
> fixed in patch 1. Were we aware that we have the operands for 'bound'
> the wrong way round according to gas?)
>
> I suppose I can hunt down which instructions might work after the first
> barely-functional patch that adds .code16, and add a test case just for
> those... and repeat for the rest of the intermediate patches. But I'm
> wondering if that's really a significant benefit compared with adding
> the full test case at the end.
>
>
It makes it more obvious which of the incremental commits are adding which
functionality.


> I split the patches up for review purposes and for bisectability in case
> it breaks anything *else* (hence fixing the test case for .code32 mode
> in the first patch, and other test cases as I went along). Nobody is
> ever expected to use .code16 before the final patch in the series.
>
> In fact, perhaps I can solve this request purely by moving the part of
> patch 2 which actually handles ".code16" to the end of the sequence.
> That way, the intermediate patches are *preparatory* work and nothing
> actually works, so it can't be tested? :)
>
>
It's nice to be able to bisect any problems with individual code16
instructions too, but I think it might be better to just split out the
bugfix level patches (as you've done with 1 for example) and get all of
those in first with the associated testcase fixups and then land the code16
patch as a hunk if we want to minimize the rewriting you have to do for
each patch.

The maximally tested mechanism would be to split up things via command line
option etc. I'm not sure it's worth it in this case as you say.

Lot of the patches do seem like they'd have been good with incremental
commits and tests added though :)


> > 5) In general the patches look pretty good. I've been working through
> > the various llvm specific things that you've called out - it's been a
> > while since I've worked on any of the assemblers as well :)
>
> Thanks a lot for looking at this.
>
>
My pleasure.

-eric


>
> --
> dwmw2
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131220/886d016e/attachment.html>