<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> 1) Comments should be full sentences including punctuation when<br>
> possible.<br>
<br>
That would be my normal style. Did I add some that weren't?<br>
<br></blockquote><div><br></div><div>A couple. Not many though.</div><div><br></div><div>// No displacement<br></div><div><br></div><div>for example.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> 2) When referencing outside sources it's often best to explain in more<br>
> detail what's going on in the particular section as if the external<br>
> reference had evaporated.<br>
<br>
Again I'm not entirely sure where you see something to provoke this<br>
comment. Perhaps in the first patch where I add:<br>
<br>
// 16-bit addressing forms of the ModR/M byte have a different encoding for<br>
// the R/M field and are far more limited in which registers can be used.<br>
if (Is16BitMemOperand(MI, Op)) {<br>
if (BaseReg) {<br>
// See Table 2-1 "16-Bit Addressing Forms with the ModR/M byte"<br>
static const int R16Table[] = { 0, 0, 0, 7, 0, 6, 4, 5 };<br>
...<br>
<br>
... and you're looking at the second comment in there? I was mostly just<br>
following the precedent where we later refer to table 2-7 in much the<br>
same way. There's a limit to how much of the table we really want to<br>
reproduce locally, surely? The interesting bit is right there in the<br>
R16Table[] array...<br>
<br></blockquote><div><br></div><div>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?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Or was there something else?<br></blockquote><div><br></div><div>That patch was the primary motivator of the comment, yes. Table 2-1 is... not fun to look at.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> 3) I appreciate the splitting up of the patch into multiple and easily<br>
> reviewable chunks.<br>
><br>
><br>
> 4) Testcases should be part of the diff that adds the functionality<br>
> (in particular the ones adding instructions for 16-bit mode).<br>
<br>
Is that mandatory? As you can see, I mostly just copied the x86-32.s<br>
file and tweaked it (and compared the output with gas to make sure I<br>
wasn't putting in any "test for the wrong output" tests like the one I<br>
fixed in patch 1. Were we aware that we have the operands for 'bound'<br>
the wrong way round according to gas?)<br>
<br>
I suppose I can hunt down which instructions might work after the first<br>
barely-functional patch that adds .code16, and add a test case just for<br>
those... and repeat for the rest of the intermediate patches. But I'm<br>
wondering if that's really a significant benefit compared with adding<br>
the full test case at the end.<br>
<br></blockquote><div><br></div><div>It makes it more obvious which of the incremental commits are adding which functionality.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I split the patches up for review purposes and for bisectability in case<br>
it breaks anything *else* (hence fixing the test case for .code32 mode<br>
in the first patch, and other test cases as I went along). Nobody is<br>
ever expected to use .code16 before the final patch in the series.<br>
<br>
In fact, perhaps I can solve this request purely by moving the part of<br>
patch 2 which actually handles ".code16" to the end of the sequence.<br>
That way, the intermediate patches are *preparatory* work and nothing<br>
actually works, so it can't be tested? :)<br>
<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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.</div><div><br></div><div>Lot of the patches do seem like they'd have been good with incremental commits and tests added though :)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> 5) In general the patches look pretty good. I've been working through<br>
> the various llvm specific things that you've called out - it's been a<br>
> while since I've worked on any of the assemblers as well :)<br>
<br>
Thanks a lot for looking at this.<br>
<br></blockquote><div><br></div><div>My pleasure.</div><div><br></div><div>-eric</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
--<br>
dwmw2<br>
<br>
</blockquote>