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

[David Woodhouse]

On Fri, 2013-12-20 at 00:11 +0000, Eric Christopher wrote:
> Couple of meta comments on the patches as I've been going through,
> these will seem like nit picking and they are, however... 
> 
> 
> 1) Comments should be full sentences including punctuation when
> possible.

That would be my normal style. Did I add some that weren't? 

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

Or was there something else?

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

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? :)

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


-- 
dwmw2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5745 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131220/c34b1bae/attachment.bin>